Make compat tests work on windows #5732
Conversation
cocreature
force-pushed
the
compat-windows
branch
11 times, most recently
from
f57ba5d to
63e1481
Compare
cocreature
changed the title
cocreature
force-pushed
the
compat-windows
branch
from
dca045c to
827c4b2
Compare
cocreature
requested review from
aherrmann-da and
garyverhaegen-da
as code owners
cocreature
force-pushed
the
compat-windows
branch
from
827c4b2 to
06a18d8
Compare
This required some changes to the daml_sdk rule since the read-only installation by the assistant breaks Bazel completely. We could only apply those changes on Windows but I think I prefer the consistency across platforms here over trying to stay close to how the SDK is installed on user machines given that the SDK installation is not something we’ve had issues with. I’ve excluded the postgresql tests for now. I don’t expect them to be particularly hard to fix but I’ve already spent almost 2 days on this and having some tests run on Windows seems like a clear improvement over running no tests on Windows :) changelog_begin changelog_end
cocreature
force-pushed
the
compat-windows
branch
from
06a18d8 to
b01a6d0
Compare
| # Note (MK): This is a fun one: Let’s say $TEST_WORKSPACE is "compatibility" | ||
| # and the argument points to a target from an external workspace, e.g., | ||
| # @daml-sdk-0.0.0//:daml. Then the short path will point to | ||
| # ../daml-sdk-0.0.0/daml. Putting things together we end up with | ||
| # compatibility/../daml-sdk-0.0.0/daml. On Linux and MacOS this works | ||
| # just fine. However, on windows we need to normalize the path | ||
| # or rlocation will fail to find the path in the manifest file. | ||
| rlocation $(realpath -L -s -m --relative-to=$PWD $TEST_WORKSPACE/$1) |
There was a problem hiding this comment.
Yes, this is a very unfortunate aspect of the runfiles mechanism. Another solution is to pass in the rootpath instead of the short path. Sadly that requires extra hoops to jump through. In this case it looks like all those are Targets and have Labels. So, you could call expand_location to obtain the $(rootpath).
There was a problem hiding this comment.
I’ll leave it for now since this seems to work and I’m scared to touch it now 😨
| stripPrefix = "sdk-{}".format(ctx.attr.version), | ||
| ) | ||
| elif ctx.attr.sdk_sha256: | ||
| ctx.download_and_extract( | ||
| output = "extracted-sdk", | ||
| output = out_dir, | ||
| # TODO (MK) Make this work on other platforms. |
There was a problem hiding this comment.
Can that todo note be removed?
| # The DAML assistant will mark the installed SDK read-only. | ||
| # This breaks Bazel horribly on Windows to the point where | ||
| # even `bazel clean --expunge` fails because it cannot remove | ||
| # the installed SDK. Therefore, we do not use the assistant to | ||
| # install the SDK but instead simply extract the SDK to the right | ||
| # location and set the symlink ourselves. |
There was a problem hiding this comment.
Hmm, isn't there a risk of these tests testing something else than what users what experience? E.g. if the DAML assistant performs some configuration at installation at some point. Would it work to remove the read-only flag after install? Maybe in a trap to avoid leaving the bazel tree in a bad state if installation fails.
There was a problem hiding this comment.
Yes there is a small risk here but after having spend way too much time trying to make this work otherwise, this seemed like the most reasonable option to me. Testing the SDK installation isn’t really a compatibility thing anyway and we do still have the actual SDK integration tests for that (at least for now).
changelog_begin changelog_end
This required some changes to the daml_sdk rule since the read-only
installation by the assistant breaks Bazel completely. We could only
apply those changes on Windows but I think I prefer the consistency
across platforms here over trying to stay close to how the SDK is
installed on user machines given that the SDK installation is not
something we’ve had issues with.
I’ve excluded the postgresql tests for now. I don’t expect them to be
particularly hard to fix but I’ve already spent almost 2 days on this
and having some tests run on Windows seems like a clear improvement
over running no tests on Windows :)
changelog_begin
changelog_end
Pull Request Checklist
CHANGELOG_BEGINandCHANGELOG_ENDtagsNOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.