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

Bump Pyodide on CI #130

Merged
merged 9 commits into from
Aug 28, 2024
Merged

Bump Pyodide on CI #130

merged 9 commits into from
Aug 28, 2024

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Aug 26, 2024

Also change to explicitely use strings.

I'm guessing this should be bumped, maybe there is an automatic way to do latest ? Also not sure if you want to test on alphas, but I guess it's always useful to catch bugs.

Also change to explicitely use strings.

I'm guessing this should be bumped, maybe there is an automatic way to
do latest ? Also not sure if you want to test on alphas, but I guess
it's always useful to catch bugs.
Comment on lines 194 to 199
if parse(pyodide.__version__) < parse("0.27"):
assert len(logs) == 2, logs
assert "does not exist" in logs[-1]
assert "does not exist" in logs[-2]
else:
assert logs == [""]
Copy link
Member

Choose a reason for hiding this comment

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

@ryanking13 Looks like a regression?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, indeed, the warning is not shown at 0.26.0...

@hoodmane
Copy link
Member

Thanks @Carreau! Why not split this into a PR which adds 0.26 which we certainly want, and a second PR that adds 0.27.0a2 so we can investigate the failure there and make an appropriate fix without blocking the addition of 0.26?

@Carreau
Copy link
Contributor Author

Carreau commented Aug 26, 2024

0.26 also has the regression, and there does not seem to be any other regression between 0.26 and 0.27.0a2, so hopefully it's not more difficult. I also see selenium Timeouts and that I'm a bit flumoxed.

@Carreau
Copy link
Contributor Author

Carreau commented Aug 26, 2024

Also any pointers on change the value of the selenium timeouts ? I think it's 600 for now, (i'm guessing seconds ?). That would help making failing test faster

@@ -61,6 +61,9 @@ def uninstall(packages: str | list[str], *, verbose: bool | int = False) -> None
# - scripts
# - entry_points
# Since we don't support these, we can ignore them (except for data_files (TODO))
logger.warning(
"skipping file '%s' that is relative to root",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"skipping file '%s' that is relative to root",
"skipping file '%s' that is relative to root", str(file),

Comment on lines 194 to 199
if parse(pyodide.__version__) < parse("0.27"):
assert len(logs) == 2, logs
assert "does not exist" in logs[-1]
assert "does not exist" in logs[-2]
else:
assert logs == [""]
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, indeed, the warning is not shown at 0.26.0...

@ryanking13
Copy link
Member

It seems like importlib changed in Python 3.12 to not list non-existing files in the distribution, that's why we are not logging the removed files when uninstalling.

Probably then, you could just remove that test (test_warning_file_removed). It isn't that useful anymore.

As pointed out by Gyeongjae Choi in 130, those file are listed since
Python 3.12 and thus this error shouls not trigger.
@Carreau
Copy link
Contributor Author

Carreau commented Aug 26, 2024

Ok, let's try. I left the warning message (I guess there are other case were files might not be a file...), but added a comment it was likely unnecessary,.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks! It seems like we are now only getting the timeout error, which is not related to this PR. I'll investigate why the timeout is happening. Anyway this PR looks good to me.

@ryanking13 ryanking13 merged commit eb8c449 into pyodide:main Aug 28, 2024
4 of 7 checks passed
@Carreau
Copy link
Contributor Author

Carreau commented Aug 28, 2024

Thanks !

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