Skip to content
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 15 commits into from
Nov 10, 2022
Merged

Clean up uses of locateRunfiles #15446

merged 15 commits into from
Nov 10, 2022

Conversation

akrmn
Copy link
Contributor

@akrmn akrmn commented Nov 4, 2022

Fixes #15443

The problem was that locateRunfiles behaves differently depending on how the current executable was built:

  • When the executable is a bazel run or bazel test target, it returns the input prefixed by the runfiles directory assigned by Bazel for that target
  • When it's a packaged app produced with bazel_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 the locateRunfiles call with the dlint.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 using locateRunfiles 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 of locateRunfiles outside of tests or compatibility/ have been replaced with locateResource, removing the workarounds described above.

EDIT: additionally, this PR adds an hlint warning recommending locateResource instead of locateRunfiles - 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 the EXECUTABLE/../resources directory).

@akrmn akrmn force-pushed the akrmn/fix-locate-dlint-yaml branch 3 times, most recently from 93c63e9 to f96f613 Compare November 4, 2022 16:15
Copy link
Contributor

@nickchapman-da nickchapman-da left a 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!

@akrmn akrmn force-pushed the akrmn/fix-locate-dlint-yaml branch 2 times, most recently from 069ae8d to bd332de Compare November 7, 2022 14:56
@akrmn akrmn force-pushed the akrmn/fix-locate-dlint-yaml branch from d0a93cc to 6056364 Compare November 7, 2022 15:25
@remyhaemmerle-da
Copy link
Collaborator

remyhaemmerle-da commented Nov 8, 2022

@garyverhaegen-da could you have a look? This looks reasonable to me, but maybe you have a different perspective.

@akrmn akrmn merged commit 06892a4 into main Nov 10, 2022
@akrmn akrmn deleted the akrmn/fix-locate-dlint-yaml branch November 10, 2022 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Daml Studio reports problem "HLint 2.3 and beyond cannot use Haskell configuration files"
3 participants