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

refactor: factor gateway data fetch and solve into a function #1793

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

Conversation

olivier-lacroix
Copy link
Contributor

@olivier-lacroix olivier-lacroix commented Aug 12, 2024

A follow-up on #1767

gateway
.query(
channels,
[Platform::current(), Platform::NoArch],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this PR, the query was done for a given Platform. however, virtual_packages were computed for the current platform. Is that discrepancy expected / normal?

Copy link
Contributor

Choose a reason for hiding this comment

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

The platform must be given because you can install packages on osx for both osx-arm64 and osx-64. The virtual packages cannot be computed for an arbitrary platform though so we are stuck with the ones for the current platform.

Copy link
Contributor Author

@olivier-lacroix olivier-lacroix Aug 12, 2024

Choose a reason for hiding this comment

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

Thanks @baszalmstra .

If this is the only use case, shouldn't we guess best_platform automatically and remove the --platform argument from the global commands?

edit: I note that the exec command does not take any --platform argument already

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can get rid of the argument thatd be great! How would you go about detecting the best_platform?

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 100% sure TBH 😆 . If the only case to handle is the osx-arm to osx, we could maybe detect the right error / empty solve when the current platform is osx-arm, and try again on the 'backup' platform? would that work? is that the only case to handle?

Copy link
Contributor

Choose a reason for hiding this comment

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

Im not entirely sure if that would work, because Im not sure which situation you could get into..

@olivier-lacroix olivier-lacroix marked this pull request as ready for review August 12, 2024 13:10
@olivier-lacroix
Copy link
Contributor Author

olivier-lacroix commented Aug 12, 2024

Progress report is not the best, as messages such as fetching repodata for environment can end up being displayed a fair few times. Is there a way to make this better?

@baszalmstra
Copy link
Contributor

Progress report is not the best, as messages such as fetching repodata for environment can end up being displayed a fair few times. Is there a way to make this better?

Yes but it requires implementing a Reporter.

@olivier-lacroix
Copy link
Contributor Author

@baszalmstra I am not sure what this entails. But it does not sound straightforward 🥲 . Is it worth it? Or should we pull the messaging out of the function and wrap it when / it needed where it is used?

@baszalmstra
Copy link
Contributor

Yeah that sounds fair! 👍

@olivier-lacroix
Copy link
Contributor Author

Yeah that sounds fair! 👍

Actually that does not work. Even with the messaging outside of the loop, the iterator is still there waiting... Reporter it is then :-)

@baszalmstra
Copy link
Contributor

Rattler-build has a custom implementation here: https://github.com/prefix-dev/rattler-build/blob/a28628bc6d29e227dffc1a64c27fa5f0b50f7dc9/src/render/solver.rs#L136

Its not great but might serve as inspiration!

@baszalmstra baszalmstra self-assigned this Aug 20, 2024
@tdejager
Copy link
Contributor

tdejager commented Sep 9, 2024

Hi guys! What is the status of this PR @olivier-lacroix @baszalmstra?

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