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

Create a new switch if not pre-installed #242

Merged
merged 1 commit into from
Aug 30, 2018

Conversation

NathanReb
Copy link

This adds a fallback when opam switch OCAML_VERSION fails, attempting to create the corresponding switch, addressing #241.

This isn't perfect though since the pre-installed switches' names differ from the compiler names. One might wonder why their CI builds takes ages and have trouble figuring out it's because their .travis.yml contains
- OCAML_VERSION=4.07.0 instead of - OCAML_VERSION=4.07.
Not sure what should be done to avoid this!

@samoht
Copy link
Member

samoht commented Aug 29, 2018

One might wonder why their CI builds takes ages

yes that's a bit annoying. One of the great thing about the current behavior is that the failure mode is pretty clear. I am wondering if we should try to compare version in bash and do the switch only if that's below 4.03 (I am not sure I really like that this solution either).

@NathanReb
Copy link
Author

NathanReb commented Aug 29, 2018

I'm not super fond of making those scripts more complex but as you pointed out the current error is better than silently falling back to switch creation unexpectedly!

If you want to add a check in the script I think it would need to be more accurate than < 4.03 since I guess some people could be interested in having variants (as in non base-compilers) in their CI.

Maybe adding a case on the actual pre-installed compiler versions to convert them to the 4.0x form would do the trick. That still means the script has to be updated when those change but I guess that's ok.

Your call!

@samoht
Copy link
Member

samoht commented Aug 30, 2018

Same here, I'm not super fond of making the script more complex.

Maybe the right solution is to control that behavior via an OPAM_SWITCH_CREATE env variable. It's a bit ugly, but at least it's explicit.

@avsm
Copy link
Member

avsm commented Aug 30, 2018

One advantage of your patch is that we can now specify a particular patch release (e.g. OCAML_VERSION=4.07.0).

How about just printing something to stdout to indicate that a custom switch is getting compiled with a note that things might be faster if the final .0 is dropped? That would be a simple change and not add yet another environment dial into the mix.

@samoht
Copy link
Member

samoht commented Aug 30, 2018

Ok let's merge this, we can see if we can make things better later.

@samoht samoht merged commit 2f44ead into ocaml:master Aug 30, 2018
samoht added a commit to samoht/opam-repository that referenced this pull request Sep 24, 2018
CHANGES:

* Set variables before installing ppas (ocaml/ocaml-ci-scripts#234, @samoht)
* Fix name of system switch when using INSTALL_LOCAL=1 (ocaml/ocaml-ci-scripts#236, @samoht)
* Fix .travis-docker.sh when opam 2 is used (ocaml/ocaml-ci-scripts#237, @gaborigloi)
* Add support for BASE_REMOTE_BRANCH (ocaml/ocaml-ci-scripts#239, @lindig)
* Create a new switch if not pre-installed (ocaml/ocaml-ci-scripts#242, @NathanReb)
* Better lint for opam 1.2 files when using opam2 (ocaml/ocaml-ci-scripts#245, @samoht)
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