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

fix: use local mapping instead of remote #1430

Merged
merged 18 commits into from
May 27, 2024

Conversation

nichmor
Copy link
Contributor

@nichmor nichmor commented May 23, 2024

This PR aims to fix the issue where the OnceCell for the loaded mapping was initialized only once during the tests and wasn't affected by project changes, which made our tests flaky.

This is not a problem for our end users who are using Pixi as a binary, but it could be a problem if it's used as a library.

@baszalmstra
Copy link
Contributor

Do you know what caused the flakyness though?

@nichmor
Copy link
Contributor Author

nichmor commented May 23, 2024

Do you know what caused the flakyness though?

We are using in this test a simple reqwest client without adding a retry and cache layer for it ( which happend in real code ). It could be that github don't respond successfully for the first time which cause the test to fail

@baszalmstra
Copy link
Contributor

Sure but if we have 500 errors or whatever I would expect the mapping process to fail with an error as well. Otherwise, we get flaky behavior.

@nichmor
Copy link
Contributor Author

nichmor commented May 23, 2024

Sure but if we have 500 errors or whatever I would expect the mapping process to fail with an error as well. Otherwise, we get flaky behavior.

let me write a test for this behaviour

@nichmor
Copy link
Contributor Author

nichmor commented May 23, 2024

problem was that we were using oncecell in custom_pypi_mapping.rs which was loaded once during the tests and reused indifferent of project defined in test.

I've moved this function in struct CustomMapping, so it will loaded depending on the project

@tdejager
Copy link
Contributor

Actually, the problem is that that thing is Static that makes it available once per binary :)

src/project/mod.rs Outdated Show resolved Hide resolved
.pypi_name_mapping_source()
.expect("mapping source should be ok")
pub fn pypi_name_mapping_source(&self) -> MappingSource {
self.mapping_source
Copy link
Contributor

Choose a reason for hiding this comment

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

DOes it even make sense to use a oncecell here? Since the mapping is already just returned from 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.

we are doing some transformation in self.manifest.pypi_name_mapping_source()
from user defined channels into rattler::Channel
so my idea was to cache it here

@nichmor nichmor requested a review from baszalmstra May 24, 2024 13:08
@ruben-arts ruben-arts merged commit 2337654 into prefix-dev:main May 27, 2024
25 checks passed
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