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

Use shorter build path to avoid long path issues on Windows #969

Merged
merged 4 commits into from Oct 15, 2019

Conversation

ManasJayanth
Copy link
Member

@ManasJayanth ManasJayanth commented Aug 27, 2019

Uses shorted build path /.esy//b

When packages need deep nested paths (like
ocaml-migrate-parsetree. See
https://gist.github.com/prometheansacrifice/7309940918d646d465d9d6abd0a5d231),
packages fail to build (despite having enabled long paths). This PR
uses a shorter path for global build cache.

@ManasJayanth ManasJayanth force-pushed the prometheansacrifice/use-shorter-global-build-path branch from 0ce6988 to aa74533 Compare August 27, 2019 18:46
@ManasJayanth ManasJayanth marked this pull request as ready for review August 27, 2019 18:47
@ManasJayanth
Copy link
Member Author

Please don't merge it just yet

When packages need deep nested paths (like
ocaml-migrate-parsetree. See
https://gist.github.com/prometheansacrifice/7309940918d646d465d9d6abd0a5d231),
packages fail to build (despite having enabled long paths). This PR
uses a shorter path for global build cache.
@ManasJayanth ManasJayanth force-pushed the prometheansacrifice/use-shorter-global-build-path branch from 8527211 to 928c68d Compare August 29, 2019 11:17
@ManasJayanth
Copy link
Member Author

Tested and works fine with OCaml Parse Migratetree

Copy link
Member

@andreypopp andreypopp left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you.

One thing I wonder is if we could keep the config for esy-build-package with the same number of options. Right now globalStorePrefix and storePath both are redundant I think.

storePath: EsyLib.Path.t,
localStorePath: EsyLib.Path.t,
disableSandbox: bool,
};
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we could keep only three paths there — project path, path for immutable builds (global store) and path for transient builds (local store)?

I think here we could determine storePath out of globalStorePrefix, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we can definitely do that. I'll be on it.

@ManasJayanth ManasJayanth changed the title Use shorter build path to avoid long path issues on Windows [WIP] Use shorter build path to avoid long path issues on Windows Aug 31, 2019
@ManasJayanth ManasJayanth force-pushed the prometheansacrifice/use-shorter-global-build-path branch from 4f01a75 to 11e08b0 Compare September 5, 2019 11:27
@ManasJayanth ManasJayanth changed the title [WIP] Use shorter build path to avoid long path issues on Windows Use shorter build path to avoid long path issues on Windows Sep 5, 2019
@jordwalke
Copy link
Member

Thanks @prometheansacrifice !

@jordwalke
Copy link
Member

Good to merge @andreypopp? This could get rid of a huge set of potential windows build failures (though I haven't seen any lately).

@ManasJayanth ManasJayanth force-pushed the prometheansacrifice/use-shorter-global-build-path branch from 2e8325e to be00486 Compare October 14, 2019 12:41
Copy link
Member

@jordwalke jordwalke left a comment

Choose a reason for hiding this comment

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

Awesome!

@jordwalke jordwalke merged commit 2334202 into master Oct 15, 2019
@ManasJayanth ManasJayanth deleted the prometheansacrifice/use-shorter-global-build-path branch October 15, 2019 12:09
@andreypopp
Copy link
Member

Sorry, missed the notification. Glad to see this merged!

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.

None yet

3 participants