Skip to content

[esy] Read dependencies in esy from installation.json #3860

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

Closed
wants to merge 2 commits into from

Conversation

ulrikstrid
Copy link
Contributor

I'm piggy backing on the custom_resolution flag that is already set when building with esy here. If we can't find the dependency in the environment variables we'll look it up in the installation.json file from the current sandbox.

@bobzhang can I make the lookup per package be lazy somehow so that we don't have to do it multiple times?

Notes:

  • This implementation assumes that the build is run with "buildsInSource": true which I think is valid and needed anyway.
  • This PR doesn't make much sense without landing some variation of [esy] Out of source compilation #3856

@ulrikstrid ulrikstrid changed the title Read dependencies in esy from installation.json [esy] Read dependencies in esy from installation.json Oct 2, 2019
@@ -106,6 +106,22 @@ let pkg_name_as_variable package =
|> fun s -> Ext_string.split s '-'
|> String.concat "_"

let rec find_dep_path ic package_name =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing you could do that might make this function simpler is make this function do the try/catch and return an option of string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

@bobzhang
Copy link
Member

bobzhang commented Oct 12, 2019

@bobzhang can I make the lookup per package be lazy somehow so that we don't have to do it multiple times?

Yeah, I think it is a good idea
The PR is small and looks nonintrusive, lgtm.
Ping me when you feel good to merge

@ulrikstrid
Copy link
Contributor Author

@bobzhang I couldn't figure out how to make the computation lazy but I moved the try/catch to a better position and I think this is good to go now.

I also rebased on master.

@cristianoc cristianoc closed this Jun 25, 2022
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.

4 participants