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

Handle 'or' in versions in a primitive manner #2840

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

filmor
Copy link
Contributor

@filmor filmor commented Oct 20, 2023

The first constraint in a chain of "or"'d constraints is taken into
account, all others are ignored. That way, there is at least some
resolution happening, which can always be overridden via rebar.config.

@filmor
Copy link
Contributor Author

filmor commented Oct 23, 2023

@ferd @tsloughter Would something like this have any chance of getting merged? It sidesteps the whole "requirement vs version" and verl discussion by just using the first condition. This would allow me to progress on my rebar_mix attempt, while still being able to just force the required version.

@ferd
Copy link
Collaborator

ferd commented Oct 30, 2023

Yeah I'd be willing to get an in-between patch where or is handled explicitly to at least reduce some amounts of glaring conflicts. I don't see a problem with that, particularly if it has tests.

@filmor filmor marked this pull request as ready for review November 2, 2023 09:41
@filmor
Copy link
Contributor Author

filmor commented Nov 2, 2023

Good, anything I should add or change before this can be merged? The failure on macOS was due to brew not getting updates, rerunning it is probably enough.

@ferd
Copy link
Collaborator

ferd commented Nov 3, 2023

Yeah I think I'd like a test that runs on versions like 1.0.1-custom-or-label or 1.0.0 and shows that it doesn't see the or in the label as a split source. I think this will otherwise let people do weird stuff with version injection.

The first constraint in a chain of "or"'d constraints is taken into
account, all others are ignored. That way, there is at least *some*
resolution happening, which can always be overridden via `rebar.config`.
@filmor
Copy link
Contributor Author

filmor commented Dec 25, 2023

Finally got around to adding tests (and "fixing" the exact issue that you may have been hinting at ;)).

@filmor
Copy link
Contributor Author

filmor commented Jan 8, 2024

@ferd The only error here is the brew doctor call which, I think, should be made into a warning instead of failing the whole run.

Copy link
Collaborator

@ferd ferd left a comment

Choose a reason for hiding this comment

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

Yep thanks for the edit, and yeah the brew thing is known breakage I need to look at. Merging this!

@ferd ferd merged commit 4763199 into erlang:main Jan 8, 2024
5 of 6 checks passed
@filmor filmor deleted the simplified-or-handling branch January 9, 2024 06:27
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.

2 participants