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

show latest major version available in gleam deps update #3876

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

Conversation

andho
Copy link

@andho andho commented Nov 22, 2024

This PR aims to solve #3843.
The sample output from that issue:

 % gleam deps update
  Resolving versions
Downloading packages
 Downloaded 2 packages in 0.01s
 
Hint: the following dependencies have new major versions available...

- wibble@2
- wobble@3

Todos

  • Handle Async: fireoff the checks and show the results at the end. Didn't make sense to do this.
  • Telemetry? Not sure it's needed for this optional part.
  • Merge version check into a single call Hexpm API doesn't have this. Went with cache instead.
  • Get the direct dependencies only

@lpil
Copy link
Member

lpil commented Dec 2, 2024

Hello! Are you still working on this one? Thanks

@andho
Copy link
Author

andho commented Dec 5, 2024

Hello! Are you still working on this one? Thanks

I was hoping to work on it this weekend but some things have come up. I can only get back to it next weekend.

@andho andho force-pushed the notify-major-version branch from 320e39f to fa633dc Compare December 6, 2024 03:33
@andho andho marked this pull request as ready for review December 6, 2024 15:25
@andho
Copy link
Author

andho commented Dec 6, 2024

Just the major version hist without any other output is weird though:
image

Helpful, but looks weird.

@andho andho force-pushed the notify-major-version branch from 300e9eb to 2f1055d Compare December 9, 2024 11:02
@inoas
Copy link
Contributor

inoas commented Dec 9, 2024

Could a flag be provided that forces a non-zero exit-code if major deps updates are available?

Would that make sense?

The idea would be to have a CI check that goes red if deps are not up to date.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Very nice! I've left some small notes inline.

]
.into_iter()
.collect(),
//requirements: HashMap::new(),
Copy link
Member

Choose a reason for hiding this comment

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

Unused code here

Copy link
Author

@andho andho Dec 12, 2024

Choose a reason for hiding this comment

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

Haha, this is embarrassing.
PS. Fixed.

let result = runtime_cache.get(package);

if let Some(result) = result {
return Ok(result.clone());
Copy link
Member

Choose a reason for hiding this comment

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

Rather than clone the entire package lets record the highest version here.

Copy link
Author

Choose a reason for hiding this comment

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

Oh! This function is originally used by the DependencyProvider. But as the major version need the same info, but to avoid multiple http calls to hexpm, I've added caching here.

I think the way to avoid cloning would be to have PackageFetcher own package (and return a ref) but that would have to touch the dependency resolution part so I didn't want to include in this PR (increased scope).

Copy link
Author

Choose a reason for hiding this comment

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

Here's an implementation with PackageFetcher owning packages and RefCounting: andho@851b9a6

Copy link
Author

Choose a reason for hiding this comment

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

Ah I just realized I never wired up the same PackageFetcher to be used in other parts of the dependency resolution so the cache is not even used. I'll see about getting that wired up soon.

Copy link
Author

Choose a reason for hiding this comment

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

PackageFetcher is now passed by reference in all instances and instantiated in list and download only.

Copy link
Member

Choose a reason for hiding this comment

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

It would be best to do that as to not clone everything. Do you think it would be complex or involved?

let Some(latest) = &hexpackage
.releases
.iter()
.map(|release| release.version.clone())
Copy link
Member

Choose a reason for hiding this comment

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

Can take a reference here and avoid cloning

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 88 to 89
.sorted_by(|a, b| b.cmp(a))
.next()
Copy link
Member

Choose a reason for hiding this comment

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

Using .max will be faster than sorting and taking the first

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.


// lazily assuming the longest version could be 8 characters. eg: 1.1234.2
// excluding qualifiers other than x.y.z
eprintln!("{name}:{padding} {v1:<8} -> {v2:<8}");
Copy link
Member

Choose a reason for hiding this comment

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

Let's render this to a string and then snapshot test the function. This function can then just print the string and not be responsible for formatting and printing

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@lpil lpil marked this pull request as draft December 9, 2024 15:55
@andho andho force-pushed the notify-major-version branch from 0781fc3 to c2e7c65 Compare December 12, 2024 16:21
@andho
Copy link
Author

andho commented Dec 13, 2024

Could a flag be provided that forces a non-zero exit-code if major deps updates are available?

Would that make sense?

The idea would be to have a CI check that goes red if deps are not up to date.

@inoas I think the flag would make more sense in a gleam deps outdated command instead.

@andho andho force-pushed the notify-major-version branch from c2e7c65 to ce0b650 Compare December 13, 2024 07:15
…cy resolution and major version resolution to reduce network calls to hexpm ♻️
@andho andho force-pushed the notify-major-version branch from 100bf86 to 839318a Compare December 13, 2024 12:54
@andho andho marked this pull request as ready for review December 13, 2024 13:03
@andho andho changed the title Draft: show latest major version available in gleam deps update show latest major version available in gleam deps update Dec 14, 2024
@andho andho requested a review from lpil December 14, 2024 13:05
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Great, thank you.

image

I gave it a try and while the information is there and useful I don't think it fits in at all with any of the other information printed. How can we show this better?

Should it be shown for all resolution or only for gleam update? If the latter it would be easier to make a good output format for it.

@andho
Copy link
Author

andho commented Dec 18, 2024

Should it be shown for all resolution or only for gleam update?

It makes sense to show for gleam update and gleam deps download

Although it was helpful when I recently did gleam build and I saw that decode package was updated to 1.0.0. But due to the execution order it would take some rearranging to get this output to come last.

I'd say for an MVP show only in gleam update and gleam deps download. I'll only have to add a flag to download fn.

@lpil
Copy link
Member

lpil commented Dec 19, 2024

Sounds great!

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.

3 participants