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

feat(manager/gleam): enable update-lockfile #31002

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SnakeDoc
Copy link
Contributor

@SnakeDoc SnakeDoc commented Aug 25, 2024

Changes

Enables lockfile updates

This should not be merged until #31000 is accepted.
#31000 has been merged, we can proceed with this PR.

Context

Supersedes #30811

The Gleam manager does not allow rangeStrategy=update-lockfile

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@@ -49,7 +49,7 @@ export async function updateArtifacts(
],
};

await exec('gleam deps download', execOptions);
await exec('gleam deps update', execOptions);
Copy link
Member

Choose a reason for hiding this comment

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

does this update all dependencies to their latest allowed version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

then this causes unwanted side effects for normal updates. update should only called when the update is a lockfile update.

@rarkins @secustor WDYT?

Copy link
Contributor Author

@SnakeDoc SnakeDoc Aug 26, 2024

Choose a reason for hiding this comment

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

Thanks @viceice, I appreciate you noticing this - and yes, you are correct this change may cause side effects. Specifically, gleam deps update will update all deps in the lock file (including transients), respecting version ranges from the package file.

Below is some context and musings regarding this specific change, hopefully they are helpful. Perhaps there is a better way to handle this?

Given the following pseudo-situation:

Package File Contents: >= 1.0.0 and < 2.0.0
Lock File Contents: 1.2.0
Current Version: 1.3.0

Running gleam deps download results in:

Package File Contents: >= 1.0.0 and < 2.0.0
Lock File Contents: 1.2.0
Current Version: 1.3.0

Running gleam deps update results in:

Package File Contents: >= 1.0.0 and < 2.0.0
Lock File Contents: 1.3.0
Current Version: 1.3.0

gleam deps update updates the entire lock file to current, as seen in dependencies.rs#L249, triggered by dependencies.rs#L117.

Explored Alternate Approaches

Gleam's lock file manifest.toml is valid TOML, however it uses a non-standard TOML format, which makes it challenging to reliably update locked versions in-place.

The options are to either throw out the default formatting, resulting in a lock file that looks very foreign to gleam users, or use gleam deps update to manage the lock file.

Using a TOML lib to parse, update, then re-stringify back to disk results in a very different manifest.toml format. This is undesirable since we will then have multiple tools fighting over the lock file format.

The least problematic approach at this time seems to be to allow gleam deps update to update the lock file.

Thoughts? Perhaps I overlooked something? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem here is if a user has multiple dependencies which are unpinned in the lock file, and Renovate creates a PR which claims to update one of them, but actually updates all of them.

The gleam manager needs a way to specify one or more dependencies to update, e.g. gleam deps update foo to update package foo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Either:

  1. Reverse engineer the lock file format (which is a bit risky), or
  2. Document that it's not supported and recommend pinning dependencies instead

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would unexpected behaviour by users and would look ( rightfully ) like a bug to them as glean would be the only package manager showing this behavior.

I see these options to move forward:

  • parse manually the lock file ( as @rarkins mentioned which is very risky and a lot of work to maintain )
  • request a scoped update command for the Gleam CLI. This is for example how PDM lock files are updated.
  • Implement as is but make it opt in as an experimental flag. That way people are aware of the limitations before actually using.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To reuse some Renovate terminology, if we merged this then it would look like it's meant to update just one dependency, but actually each PR would be a "lock file maintenance". I think the confusion/misleadingness would be too much

Copy link
Member

Choose a reason for hiding this comment

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

why not stay on deps download like now when it's a normal update? so only necessary changes occur to the manifest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a bad idea for the interim. I will do some testing and see if the behavior is acceptable.

I have also tagged this issue gleam-lang/gleam#2170, and will work on getting this feature into gleam. That will provide the best permanent solution, but will take unknown time/effort.

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.

4 participants