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

Format upgrade fix: remove missing switches from config file #4715

Merged
merged 17 commits into from
Jun 21, 2021

Conversation

rjbou
Copy link
Collaborator

@rjbou rjbou commented Jun 16, 2021

@rjbou rjbou added this to the 2.1.0~rc2 milestone Jun 16, 2021
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Possible message tweaks

src/state/opamFormatUpgrade.ml Outdated Show resolved Hide resolved
src/state/opamFormatUpgrade.ml Outdated Show resolved Hide resolved
@rjbou rjbou changed the title Format upgrade fix: remoe missing switche from config file Format upgrade fix: remove missing switche from config file Jun 16, 2021
@rjbou rjbou requested review from dra27 and AltGr June 17, 2021 09:08
@rjbou rjbou changed the title Format upgrade fix: remove missing switche from config file Format upgrade fix: remove missing switches from config file Jun 17, 2021
Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

I am not sure about the unrecorded switches part though, we'd need to scan the whole disk for it to be consistent.

src/state/opamFormatUpgrade.ml Outdated Show resolved Hide resolved
src/state/opamFormatUpgrade.ml Outdated Show resolved Hide resolved
@rjbou rjbou requested a review from AltGr June 18, 2021 08:11
| exception (OpamPp.Bad_version _ as e) ->
match OpamFile.Config.raw_root_version (OpamPath.config root) with
| None -> raise e
| Some _ ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should it be constrained to root version 2.1~rc/2.1 ?

Copy link
Member

Choose a reason for hiding this comment

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

I think so - doesn't this become a problem in future if it gets opam-root-version: "2.2"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With a 2.2 opam root, the local switch upgrade won't be triggered, yes.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, of course, the whole thing is in the exception for an incorrect version.

Copy link
Member

Choose a reason for hiding this comment

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

Since this fix is only necessary for intermediate dev versions, I'd say we remove it from 2.2 or at least 2.3: it'll keep the upgrade code more maintainable and we can reasonably assume anyone who tried one of the affected versions switched to the release in the meantime ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's unregistered local switch, they can stay around for a while without noticing

Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

Only minor (mostly style and code doc) comments

src/state/opamStateConfig.ml Outdated Show resolved Hide resolved
| exception (OpamPp.Bad_version _ as e) ->
match OpamFile.Config.raw_root_version (OpamPath.config root) with
| None -> raise e
| Some _ ->
Copy link
Member

Choose a reason for hiding this comment

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

Since this fix is only necessary for intermediate dev versions, I'd say we remove it from 2.2 or at least 2.3: it'll keep the upgrade code more maintainable and we can reasonably assume anyone who tried one of the affected versions switched to the release in the meantime ?

src/state/opamStateConfig.mli Outdated Show resolved Hide resolved
@@ -476,6 +484,65 @@ let run_test ?(vars=[]) ~opam t =
List.fold_left
(fun vars (v, r) -> (v, r) :: List.filter (fun (w, _) -> v <> w) vars)
vars bindings
| Cat files ->
let print_opamfile header file =
Copy link
Member

Choose a reason for hiding this comment

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

This probably deserves to be a top-level function 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it will be removed from run.ml to be an outside program

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The layout update should not fail even if the installed-switches are incorrect
3 participants