-
Notifications
You must be signed in to change notification settings - Fork 40
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
setup-ocaml v2 #66
setup-ocaml v2 #66
Conversation
d5ff254
to
43c468b
Compare
1fd9c62
to
de3fc7f
Compare
README.md
Outdated
- 4.10.1 | ||
- 4.09.1 | ||
- 4.08.1 | ||
- ocaml-base-compiler.4.12.0 |
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.
Wouldn't the previous version numbers work as well?
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.
For now, no, but we can support it. (I didn't have much incentive to support it, so I did not implement that out to keep the code simple.)
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 think keeping the current format is beneficial for users: it's short, which helps users to tests the packages on multiple versions; and it's the same as in v1, which helps currents users to migrate.
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.
It's understandable that it makes the migration easier. However, I want to be as close as possible to the new package naming style introduced in 4.12: https://ocaml.org/releases/4.12.0.html (like: --package=ocaml-variants.4.12.0+options,ocaml-option-flambda,ocaml-option-nnpchecker
)
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.
@psafont I just re-added semver style version matching support! (For base compilers, not variants, you can just use 4.12.x
. This greatly reduces maintenance costs.)
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.
@Gbury Perhaps you are also interested in this change.
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.
good to know, ^^
README.md
Outdated
- run: opam install . --deps-only | ||
|
||
- run: opam exec -- dune build | ||
|
||
- run: opam exec -- dune runtest |
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'd like to see a testing workflow by default, this is the oportunity to "recommend" a workflow to ocaml newcomers:
- run: opam install . --deps-only | |
- run: opam exec -- dune build | |
- run: opam exec -- dune runtest | |
- run: opam install . --deps-only --with-test | |
- run: opam exec -- dune build | |
- run: opam exec -- dune runtest |
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 was thinking about the same thing, but from v2, opam pin and depext run automatically (You can optionally disable it), so I intentionally removed it to keep the example very minimal. (If partial revert the changes, we have to add --with-test
to the opam-depext-flanges
input.)
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.
Otherwise, we can disable these functionalities by default. Because it's mainly for large projects.
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.
If it can be enabled I'd like to see how to use these advanced option.
Usually for testing there are no depexts needed, so having the tests would still work. Starting with opam 2.1.0 would the depexts be automatically installed with opam install
?
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.
Definitely. Users should see what they are. Yes, in general it's not required, but it's often needed, so I'd like to put it to avoid confusion.
Starting with opam 2.1.0 would the depexts be automatically installed with
opam install
?
IIRC, opam-depext as a package will be deprecated and will just be bundled with opam. CC: @rjbou
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.
yes, depext is integrated in opam 2.1 (not yet release, soon a release candidate), but opam 2.0 still uses opam-depext
plugin. Ftm it is frozen, and once 2.1 released it will be slowly deprecated.
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.
By the way, if setup-ocaml try to install the opam-depext package with opam 2.1, how opam behave? Returns an error? Or install the old, unbundled depext?
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.
@psafont I just applied the suggestion!
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.
The package is still available, so it will installs it, and can uses it, but its content won't be update (except major issue/security bug). With the new integrated system, there is no more the need to call opam depext ...
, it's completely transparent. So calling opam depext
with opam 2.1 will behave as opam 2.0 : install plugin depext and calls it.
The main inconvenience is that you will do an external dependency check twice.
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.
With the new integrated system, there is no more the need to call opam depext ..., it's completely transparent.
Whoa! This is much cooler than I thought!
I'm happy to know that it won't break. I just wanted to make sure that OCaml workflows around the world would not break unless I made changes for opam 2.1 immediately after the release.
Thank you, as always, for all your fantastic work on opam!
e7987f6
to
7ff6869
Compare
7eccfc5
to
081ee5e
Compare
e35b985
to
e1ef61a
Compare
dc1e924
to
a17f0fc
Compare
Signed-off-by: Sora Morimoto <sora@morimoto.io> Co-Authored-By: David Allsopp <david.allsopp@metastack.com> Co-Authored-By: Lucas Pluvinage <lucas@tarides.com>
Signed-off-by: Sora Morimoto <sora@morimoto.io> Co-Authored-By: Pau Ruiz Safont <pau.safont@citrix.com>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
This was actually all rewritten from scratch, and in the process many changes were made to make things better and better. (Mainly for Windows and Build Performance.)
Now the action does the following:
opam-depext
is installed, butdepext-cygwinports
is installed as wellPost:
The reason for not caching opam stuff in the post stage (more precisely, why you can't) is due to the size of the cache and repeatability. They should be cached immediately after initialisation to minimize the size of the cache.
Inputs
ocaml-compiler
: The OCaml compiler packages to initialise. The packages must be separated by the comma.opam-repository
: The URL of the repository to fetch the packages from.opam-pin
: Enable the automation feature for opam pin.true
opam-depext
: Enable the automation feature for opam depext.true
opam-depext-flags
: The flags for the opam depext command. The flags must be separated by the comma.opam-local-packages
: The local packages to be used byopam-pin
oropam-depext
. See@actions/glob
for supported patterns.*.opam
opam-disable-sandboxing
: Disable the opam sandboxing feature.false
dune-cache
: Enable the dune cache feature.false
cache-prefix
: The prefix of the cache keys.v1
github-token
: DO NOT SET THIS. The API token to be used by the action to avoid rate-limiting when calling the GitHub API internally.${{ github.token }}
Follow-up to misunderstandable parts:
opam-disable-sandboxing
is always meaningless on Windows.opam-local-packages
actually allows complex glob patterns that span multiple lines:See
@actions/glob
for supported patterns.In what kind of cases should I use
cache-prefix
?Do I need to create an API token, set it as a repository secret, and pass it to the action via
github-token
?!Comparison
Set-up
Here is a simple benchmark. However, this is not really a fair benchmark. In v2, if the cache wasn't hit, the time spent creating the cache and uploading it to the server is included, so they are a bit longer than the actual set-up time.
In addition, the set-up time on macOS is very unstable. The Actions team is currently working to fix it, but due to an issue such as actions/runner-images#2707, the set-up time for each run is completely unreliable.
Others
Here is a result of benchmarking several times on a medium-sized repository using ubuntu-latest VM.
opam install . --deps-only --with-test --with-doc
opam exec -- make build
opam exec -- make test
Relevant issues and pull requests
General
Issues
Pull requests
Cache
Issues
Dune cache
Issues
Pull requests
Acknowledgement: This work was done under great supervision of @dra27.