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

Filtering: Include everything by default #98

Merged

Conversation

martinRenou
Copy link
Contributor

@martinRenou martinRenou commented Jun 5, 2024

Towards fixing jupyterlite/xeus#66

This PR updates the empack config to include everything by default, and only extra exclude rules should be added for packages.

I believe the original filtering implementation is good, it's our usage of the config that was not user-friendly nor practical.

@martinRenou martinRenou added the enhancement New feature or request label Jun 5, 2024
@martinRenou
Copy link
Contributor Author

@DerThorsten Would you like to have a look? It's unclear to me why the CI fails, it looks like there is a timeout error on the playwright tests

@martinRenou
Copy link
Contributor Author

It seems xeus-python cannot start when using this empack branch, this may be why the playwright test fails

Screenshot from 2024-06-05 17-54-47

@DerThorsten
Copy link
Collaborator

It seems xeus-python cannot start when using this empack branch, this may be why the playwright test fails

Screenshot from 2024-06-05 17-54-47

I will debug this tomorrow, thanks for starting this!

@martinRenou
Copy link
Contributor Author

I'm reworking the implementation to completely ditch the include_patterns parameter

@DerThorsten
Copy link
Collaborator

I'm reworking the implementation to completely ditch the include_patterns parameter

So we do no filtering by default, and you can just specify what to remove, and that's it?

@martinRenou
Copy link
Contributor Author

Yep, that was what I implemented initially but I've done it in the default config. I just reworked the implementation to do this in the Python code. That way if users are already shipping their own config they will get the fix too.

@martinRenou
Copy link
Contributor Author

I will debug this tomorrow, thanks for starting this!

I should be able to debug this, I'm looking at the diff between the packed env on main and the packed env on this branch. It seems to differ a lot because I'm now including lots of stuff which may prevent it to start.

@martinRenou martinRenou force-pushed the include_everything_by_default branch from ff2c419 to 3a3b209 Compare June 6, 2024 09:50
@martinRenou
Copy link
Contributor Author

The CI failure should be fixed by emscripten-forge/pyjs#67

@martinRenou martinRenou force-pushed the include_everything_by_default branch from f481aaf to 9ccc0d7 Compare June 17, 2024 10:33
@martinRenou martinRenou marked this pull request as ready for review June 17, 2024 10:39
@martinRenou martinRenou force-pushed the include_everything_by_default branch from 9ccc0d7 to bd5a4a8 Compare June 17, 2024 10:40
@martinRenou martinRenou requested a review from DerThorsten June 17, 2024 10:45
@martinRenou martinRenou merged commit 4eddbbe into emscripten-forge:main Jun 17, 2024
4 checks passed
@martinRenou martinRenou deleted the include_everything_by_default branch June 17, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants