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

Robust build infrastructure #1185

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Robust build infrastructure #1185

merged 3 commits into from
Sep 27, 2023

Conversation

dhil
Copy link
Member

@dhil dhil commented Sep 26, 2023

This patch makes the build infrastructure for Links more robust. Previously, the build infrastructure would assume that it is running in an environment populated by OPAM. This assumption is too optimistic; for example a Docker container will give you a blank environment by default. To make it easier to install Links in such environments we now prefix dune build commands with opam exec. This command resolves the installation directory of dune and makes sure it is invoked with a suitable environment.

Furthermore, the gen_linkspath.exe install helper would only test whether Links is being built by OPAM, and if that was not the case then it would assume Links was being built from sources in a local checkout of the git repository. This assumption is a tad optimistic too. So now, if we cannot detect either an OPAM installation or the git repository, then we fall back to a best effort guess based off the current working directory.

This patch makes the build infrastructure for Links more
robust. Previously, the build infrastructure would assume that it is
running in an environment populated by OPAM. This assumption is too
optimistic; for example a Docker container will give you a blank
environment by default. To make it easier to install Links in such
environments we now prefix dune build commands with `opam exec`. This
command resolves the installation directory of dune and makes sure it
is invoked with a suitable environment.

Furthermore, the `gen_linkspath.exe` install helper would only test
whether Links is being built by OPAM, and if that was not the case
then it would assume Links was being built from sources in a local
checkout of the git repository. This assumption is a tad optimistic
too. So now, if we cannot detect either an OPAM installation or the
git repository, then we fall back to a best effort guess based off the
current working directory.
@dhil dhil requested review from frank-emrich and thwfhk September 26, 2023 09:37
Makefile Show resolved Hide resolved
Copy link
Contributor

@thwfhk thwfhk left a comment

Choose a reason for hiding this comment

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

LGTM. Tested successfully in a docker container.

@frank-emrich
Copy link
Contributor

I don't quite understand the need for opam exec. To me, it seems like the only situation where dune <foo> fails but opam exec dune -- <foo> succeeds is when the user didn't run eval $(opam env). Is that correct? I'm not opposed to using opam exec, but I'd like to understand the situation better.

@dhil
Copy link
Member Author

dhil commented Sep 26, 2023

I don't quite understand the need for opam exec. To me, it seems like the only situation where dune <foo> fails but opam exec dune -- <foo> succeeds is when the user didn't run eval $(opam env). Is that correct? I'm not opposed to using opam exec, but I'd like to understand the situation better.

That's correct. It is quite annoying having to do this all the time in, say, a Docker container.

@dhil
Copy link
Member Author

dhil commented Sep 27, 2023

@thwfhk you can merge this PR when you have a minute (remember to choose the strategy "squash and merge").

@dhil
Copy link
Member Author

dhil commented Sep 27, 2023

I don't quite understand the need for opam exec. To me, it seems like the only situation where dune <foo> fails but opam exec dune -- <foo> succeeds is when the user didn't run eval $(opam env). Is that correct? I'm not opposed to using opam exec, but I'd like to understand the situation better.

That's correct. It is quite annoying having to do this all the time in, say, a Docker container.

I can expand a little upon what I said. This PR ought to remove some of the friction that you would otherwise experience when wanting to package Links as an artifact for a artifact evaluation submission. The build infrastructure is more portable as a result of this PR.

@thwfhk thwfhk merged commit b2047b7 into links-lang:master Sep 27, 2023
9 checks passed
@dhil dhil deleted the robust-build branch September 27, 2023 17:34
@dhil
Copy link
Member Author

dhil commented Mar 25, 2024

I think @thwfhk is correct. This PR solved the issue with Docker containers, I had simply forgotten about it!

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.

3 participants