fix the version range parser to support and/or version constraints#2974
fix the version range parser to support and/or version constraints#2974ferd merged 10 commits intoerlang:mainfrom
Conversation
Instead of calling ec_semver directly, all version-related calls go through this module; This makes it easy to swap later when hex_core exposes an appropriate replacement in the future. It was not possible to use `verl` or other libraries here, since rebars/erlang_commons parsing was different in a number of edge cases: - rebar allows version bounds with 2 components, like `< 1.0` or `== 1.2`. - rebar allows approximate bounds with more than 3 components, like `~> 1.5.6-rc.0` To continue to support these, the new module also uses ec_semver under the hood, but extends the custom constraint parser to also support `and` and `or` constraints used by Elixir or Gleam. Constraints are modelled as a match function without a pure data representation. This has to be assumed to be an implementation detail that might change in the future when a need arises.
Notable changes include: - find_highest_matching_ now just calls resolve_version_ with a pessimistic constraint - cmp* variants are gone - is_valid get removed in favour of handling the error case after trying to parse
ferd
left a comment
ferd
left a comment
There was a problem hiding this comment.
First of all, sorry for not reviewing this quicker.
This seems to be doing a decent job at supporting constraints, and I've only had a few minor nitpicks in terms of suggestions and a couple of extra unit tests.
One thing I've also noticed is that this broke our shelltests, but only one one scenario:
:bad_vsn_error/bad_vsn_error.test: [Failed]
Command:
TERM=dumb rebar3 compile
--- Expected
+++ Got
+ 2 ===> Uncaught error in rebar_core. Run with DIAGNOSTIC=1 to see stacktrace or consult rebar3.crashdump
+ 3 ===> When submitting a bug report, please include the output of `rebar3 report "your command"`
- 4 ===> Dep fake has invalid version >x.y.z
The test is defined at https://github.com/tsloughter/rebar3_tests/blob/master/bad_vsn_error/bad_vsn_error.test and you can see the invalid version specified at https://github.com/tsloughter/rebar3_tests/blob/b2ec3fc5f443ee26d9f53da503750ef11d19c7b3/bad_vsn_error/rebar.config#L2
What this says is that we expected a failure with an error message, but with this patch set, what we got instead was an uncaught error that led to a full rebar3 failure. This means we've got a specific error that is not handled properly but used to be.
This feels like this is almost there, this is a good piece of work!
|
Hi! No problem, I know I've been just kind of leaving this without further explanation. I didn't know about those shelltests; I think I removed parsing the constraint early from this function not realizing that the loop will break if I did. Anyways should be fixed now :) Thank you! |
ferd
left a comment
ferd
left a comment
There was a problem hiding this comment.
I think we're good to go as soon as the test runs are over. This is pretty nice! Thanks for the contribution :)
ferd
changed the title
|
Thank you! |
|
🥳 🥳 🥳 |
4 participants
Extends the custom constraint/requirement parser to be able to deal with
andandorversion ranges as they are used by Elixir and Gleam, like>= 1.2.3 and < 2.0.0.I first tried to use one of the discussed libraries, but they all had some differences that would've broken some existing tests; there's also some discussion on the previous attempts and the commented out tests in the other PR (#2785). If breaking those cases is acceptable then maybe switching to a library would be an option too. I did change all the places that called directly into
ec_semverto go through the newrebar_semvermodule instead, so it should be easier to swap in the future.This does look at the full constraint, but I figured since rebar has to loop over all available versions anyways trimming them out would be a lot of work for only a marginal performance improvement. Keeping them is likely safer and not that much slower. I've also added some tests for the version range matching, so hopefully everything stays in order.
I did not know which formatting you used here since I'm usually not an erlanger, my editor (Zed) would've done something completely different. I tried to match it manually, but if someone can point me to the formatter to use I can run that.
closes #2364
~ yoshie 💜