-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up uses of locateRunfiles
#15446
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akrmn
requested review from
basvangijzel-DA,
remyhaemmerle-da and
dylant-da
as code owners
November 4, 2022 16:04
akrmn
force-pushed
the
akrmn/fix-locate-dlint-yaml
branch
3 times, most recently
from
November 4, 2022 16:15
93c63e9
to
f96f613
Compare
akrmn
force-pushed
the
akrmn/fix-locate-dlint-yaml
branch
from
November 4, 2022 16:19
f96f613
to
12f70c3
Compare
nickchapman-da
approved these changes
Nov 7, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a couple of questions, but LGTM.
Thanks!
compiler/scenario-service/client/src/DA/Daml/LF/ScenarioServiceClient/LowLevel.hs
Outdated
Show resolved
Hide resolved
compiler/scenario-service/client/src/DA/Daml/LF/ScenarioServiceClient/LowLevel.hs
Outdated
Show resolved
Hide resolved
akrmn
requested review from
garyverhaegen-da and
ray-roestenburg-da
as code owners
November 7, 2022 13:34
akrmn
force-pushed
the
akrmn/fix-locate-dlint-yaml
branch
2 times, most recently
from
November 7, 2022 14:56
069ae8d
to
bd332de
Compare
akrmn
force-pushed
the
akrmn/fix-locate-dlint-yaml
branch
from
November 7, 2022 15:25
d0a93cc
to
6056364
Compare
@garyverhaegen-da could you have a look? This looks reasonable to me, but maybe you have a different perspective. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #15443
The problem was that
locateRunfiles
behaves differently depending on how the current executable was built:bazel run
orbazel test
target, it returns the input prefixed by the runfiles directory assigned by Bazel for that targetbazel_tools/packaging/packaging.bzl
,locateRunfiles
ignores the argument and returns the"resources"
directory, a sibling of the packaged executable.One of the changes from #15290 was using a rules file instead of a rules directory expected to contain a
dlint.yaml
file. Since the tests always use runfiles, they worked fine, but in Daml Studio it meant thelocateRunfiles
call with thedlint.yaml
file returned a directory.We'd had similar problems before due to the unexpected behavior of
locateRunfiles
, see #8278 (comment). Each time we resorted to workarounds usinglocateRunfiles
to find a base directory and then appending the filename - which only worked because in packaged apps the file was stored directly in the"resources"
directory.The fix proposed here makes the two resolution mechanisms explicit by introducing a new function
locateResource
which takes one path to use for each mechanism. All the uses oflocateRunfiles
outside of tests orcompatibility/
have been replaced withlocateResource
, removing the workarounds described above.EDIT: additionally, this PR adds an hlint warning recommending
locateResource
instead oflocateRunfiles
- which can be disabled when the codepath is never used by a packaged application. Also,locateRunfiles
now simply crashes when used from a packaged application (instead of returning theEXECUTABLE/../resources
directory).