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

Carmel::App adds version requirements incorrectly #33

Closed
karenetheridge opened this issue May 5, 2022 · 6 comments
Closed

Carmel::App adds version requirements incorrectly #33

karenetheridge opened this issue May 5, 2022 · 6 comments

Comments

@karenetheridge
Copy link

I have a cpanfile with a number of versioned dependencies in it, all expressed just as "requires 'module', '1.23';" (i.e. "give me at least this version").
I updated the dependency of one module to a version that itself has a prereq for another module
already mentioned in the cpanfile, but which specifies an earlier version, and I got an error:

e.g.

requires 'Foo', '1.000';
requires 'Foo::Bar', '0.20';    # Foo::Bar 0.20 contains a prereq for Foo 1.001

When running carmel install, the error is:

Can't merge requirements for Foo: '1.001' and '== 1.000' at /carmel/local/lib/perl5/Menlo/Dependency.pm line 53.

I believe this is coming from Carmel::App line 137:

$want_version = $ver > 0 ? "== $ver" : $ver;

This should be adding the version as ">= $ver", not "== $ver".

@miyagawa
Copy link
Owner

miyagawa commented May 5, 2022

This is an expected behavior using the snapshot file. Carmel tries to preserve the decision made in the snapshots file, and will not update the dependencies until the user explicitly asks for it.

You should either use carmel update to resolve the dependencies. You could also update cpanfile to "acknowledge" the dependency bump for that module.

Update: this is not an expected behavior, but is a corner case issue when the build artifact cache is empty. See below.

@miyagawa
Copy link
Owner

miyagawa commented May 6, 2022

I investigated this further and cannot reproduce this locally in a regular workflow as you described.

It turns out, this seems to me an edge case bug that could occur if:

  • you haven't run carmel install before, so your build artifact repo (~/.carmel) is empty
  • you update your cpanfile in the repo, then run carmel install (for the first time)

With this, carmel tries to find previously used version specified in the snapshot, but then pass it on to the cpanfile, so that cpanm run for the updated module could merge the dependencies correctly.

When the artifact repo is empty, the missing callback you quoted runs, and applies the exact match == . I believe that the exact match is here so that carmel install on the first run (without modified cpanfile) could pull the correct version from CPAN. (ae60a6d)

I suspect you shouldn't have seen this issue you run carmel install once (without modified cpanfile), then modify the cpanfile and run carmel install after that. That way Carmel can successfully locate the currently-pinned version, but allows merging the dependency upon your sub-dependency update.

That said the bug is a bug and I will keep this open.

There're several workarounds:

  • run carmel install first before modifying cpanfile (recommended)
  • run carmel inject Foo to pull the target module before running carmel install, so that Carmel should be able to locate it in the artifact cache.
  • Manually update cpanfile to acknowledge the update in transient dependency

miyagawa added a commit that referenced this issue May 6, 2022
Previously, the version is pinned to the version found in snapshot with
the exact match (==) so that it can be pulled from BackPAN using MetaCPAN
search, but that's not necessary anymore since the snapshot is now passed
to the cpanm as --mirror-index, so all the dists in the snapshot are
already preferred, while allowing the updates with a submodule update.
@miyagawa
Copy link
Owner

miyagawa commented May 6, 2022

8ea6b68

This commit will fix this particular problem, but with one caveat: as I noted in the comment, the fix will only work half the time due to perl's hash randomization, because depending on the order, cpanm could still encounter a conflicting version requirement.

This is not a bug in Carmel, and is a common issue in cpanm and Carton as well.

It might be rare for you to encounter this with Carton because with Carton you either a) don't have ./local so you pull everything from CPAN (the same if you run carton update) or b) you have ./local that satisfies the current version of the snapshot (same if you run carmel install first before modifying the cpanfile)

Ideal fix would be that we should be able to hint the order of dependency resolution based on the order of dependencies written in cpanfile. Until then, the workaround is:

  • run carmel install first, without making a change in cpanfile so you get the build artifacts in sync with the snapshot. That way, the updated dependency can pull the new dependency without having conflicts in the cpanm run.
  • run carmel install until it gets the order in a way it can resolve them without conflicts
  • update cpanfile to ignore the version in the snapshot file
  • run carmel update to ignore the snapshot file entirely

@miyagawa
Copy link
Owner

miyagawa commented May 6, 2022

I wonder if carmel update Foo can be written in a way that:

  • remove the entry of Foo in snapshot
  • run carmel inject Foo

so it will pull the latest version of Foo to update the version in the snapshot without affecting the others.

@miyagawa
Copy link
Owner

miyagawa commented May 6, 2022

I merged the above commit to master, but turns out xt/cli/core.t fails with this change, where the test was testing to make sure HTTP::Tiny (core module) can be pinned to a lower version than the version shipped with perl itself.

Previously, the == pinning was allowing cpanm to explicitly pull that version. With the removal of this, cpanm basically skips it because the core version already satisfies the requirement.

This is clearly a use case that I can consider to stop supporting, but at least there was a reason for that pinning. I added b8810c5 to show that it can be worked around if you run inject HTTP::Tiny@0.056 to populate the build artifact.

@miyagawa
Copy link
Owner

miyagawa commented May 6, 2022

The change is released to CPAN as v0.1.40.

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

No branches or pull requests

2 participants