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 the provided OCaml version when a match is not found #275

Closed
wants to merge 3 commits into from
Closed

Use the provided OCaml version when a match is not found #275

wants to merge 3 commits into from

Conversation

favonia
Copy link
Contributor

@favonia favonia commented Oct 2, 2021

I was so excited about 4.13.1, until ocaml/setup-ocaml refused to run the test because it could not find the matched releases from the repo ocaml/ocaml. (At this point of time, 4.13.1 has not been "released" on GitHub yet.) This PR will make ocaml/setup-ocaml issue a warning and proceed with the given version number, instead of using undefined as the OCaml version which leads to mysterious error messages. In sum, this PR will

  1. add a check to prevent an unexpected undefined, and
  2. use the user input as the fallback version of OCaml when a match could not be found.

@favonia favonia marked this pull request as ready for review October 2, 2021 10:53
@dra27
Copy link
Member

dra27 commented Oct 2, 2021

This is a good idea, thanks - the tag is indeed pushed to GitHub, but the release hasn't yet been made.

However, do you have code which has class row in it? If not, 4.13.1 is no more exciting than 4.13.0! In general, in your workflows you want 4.13.x (i.e. select the latest 4.13 release) - new features are not added in point releases (the only exception was 4.02.2, and it's highly unlikely that will happen again), so point releases are only exciting if you're actively affected by bug-fixes.

@favonia
Copy link
Contributor Author

favonia commented Oct 2, 2021

This is a good idea, thanks - the tag is indeed pushed to GitHub, but the release hasn't yet been made.

However, do you have code which has class row in it? If not, 4.13.1 is no more exciting than 4.13.0! In general, in your workflows you want 4.13.x (i.e. select the latest 4.13 release) - new features are not added in point releases (the only exception was 4.02.2, and it's highly unlikely that will happen again), so point releases are only exciting if you're actively affected by bug-fixes.

Fair enough. I should have said "I was excited about 4.13 and when I got time it was already 4.13.1." :-)

@smorimoto
Copy link
Member

smorimoto commented Oct 2, 2021

semverVersion also accepts range specification, so it may return >=4.13.0 <4.13.1 as is. So the action can attempt to initialise switch with ocaml-base-compiler.>=4.13.0 <4.13.1, which is not valid package specification.

@favonia
Copy link
Contributor Author

favonia commented Oct 2, 2021

semverVersion also accepts range specification, so it may return >=4.13.0 <4.13.1 as is. So the action can attempt to initialise switch with ocaml-base-compiler.>=4.13.0 <4.13.1, which is not valid package specification.

I was not sure what I should do in this case when creating the current hack. How about minVersion?

@smorimoto
Copy link
Member

I think it's enough to explicitly fail, since the exact version is basically used at the user's responsibility. Namely, like #276.

@favonia
Copy link
Contributor Author

favonia commented Oct 2, 2021

I think it's enough to explicitly fail, since the exact version is basically used at the user's responsibility. Namely, like #276.

Okay. I will commit an upgraded hack that calls semver.clean. If semver.clean fails, then the action would stop. I might steal the error message from #276.

Also use `semver.maxSatisfying` to clean up the code.
@smorimoto
Copy link
Member

semver.clean is not needed; Because semver.satisfies searches for a list of base compilers that are actually available. So the fact that no version is found means that even if you return the version you did semver.clean, it will just lose the next round that create the opam switch.

@favonia
Copy link
Contributor Author

favonia commented Oct 2, 2021

semver.clean is not needed; Because semver.satisfies searches for a list of base compilers that are actually available. So the fact that no version is found means that even if you return the version you did semver.clean, it will just lose the next round that create the opam switch.

I am sorry but it seems that you missed the essence of this PR: I wanted to use OCaml 4.13.1, and it was not released on GitHub, and I did not want to wait for the OCaml team to create a release on GitHub when I could already run opam switch create 4.13.1. What you said is normally true, but not for 4.13.1 now. Your position seems to be against the whole idea motivating this PR, suggesting that I should simply wait instead of introducing this fallback. I respect this position, but if that is the case, I should just abandon this PR instead of trying to improve it.

@smorimoto
Copy link
Member

Oops! I thought I wrote a comment, but it actually didn't...! I'm sorry. I opened #274 (Sorry for the unclear title!) yesterday that is not dependent on the GitHub releases. Combining #274 and #276 will solve the problem.

@favonia
Copy link
Contributor Author

favonia commented Oct 2, 2021

Oops! I thought I wrote a comment, but it actually didn't...! I'm sorry. I opened #274 (Sorry for the unclear title!) yesterday that is not dependent on the GitHub releases. Combining #274 and #276 will solve the problem.

Got it. Well, then what remains in this PR is to use semver.maxSatisfying instead of the current code. I would be satisfied if #276 is further cleaned up with semver.maxSatisfying and then both #274 and #276 are merged.

@smorimoto
Copy link
Member

Okay, I just updated the PR!

@favonia
Copy link
Contributor Author

favonia commented Oct 2, 2021

Closed in favor of #274 + #276.

@favonia favonia closed this Oct 2, 2021
@favonia favonia deleted the accept-any-version branch October 2, 2021 18:32
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