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

MANIFEST: Graft labextension and exclude node_modules #741

Merged

Conversation

martinRenou
Copy link
Contributor

This PR also commits yarn.lock which should be commited

@mwouts
Copy link
Owner

mwouts commented Feb 11, 2021

Thank you again @martinRenou ! I am so sorry to bother you with this...

Just one thing, I am not sure the exclusion works, at least when I build locally I get again a very large .tar.gz file.
Did you get a different outcome?

I'd prefer not to take you more time on this - in the next days I'll try to understand how the MANIFEST.in works, and also I'll had a few tests on the CI... to check the size of the package, and the presence of the two extensions as we do for the conda package.

@martinRenou
Copy link
Contributor Author

martinRenou commented Feb 12, 2021

No worries really :)

Just one thing, I am not sure the exclusion works, at least when I build locally I get again a very large .tar.gz file.
Did you get a different outcome?

I also had this issue. And looking at the logs, I understood that there was cached information under the jupytext.egg-info which was still containing false instructions. I think it was coming from SOURCES.txt.

So if you remove this egg-info directory or clean your local copy with git clean -fdx (be careful this will remove all the file untracked by git, so if you have Notebooks/files that you need there that are not tracked don't run this command) it should work.

@mwouts
Copy link
Owner

mwouts commented Feb 13, 2021

Hi @martinRenou , I've been trying to add a few tests on the CI in this branch: https://github.com/mwouts/jupytext/tree/martinRenou-manifest_graft_labext_exlude_node_modules

I think that the first test does detect node modules in the package.

Maybe we should do what you suggested above, i.e. remove the .egg-info on the CI? Can we build the package in two steps?

Also I'd be interested in having your input for testing presence of the extensions when Jupytext is installed, do you know which folder I could test?

Thanks Martin! 👋

@martinRenou
Copy link
Contributor Author

I think that the first test does detect node modules in the package.

Indeed. I must have forgotten BUILD_JUPYTERLAB_EXTENSION=1, which meant I had no generated node_modules which might be the reason I didn't see it locally.

Maybe we should do what you suggested above, i.e. remove the .egg-info on the CI?

This will not work. My PR is wrong. I'll do it differently.

@martinRenou
Copy link
Contributor Author

martinRenou commented Feb 15, 2021

Also I'd be interested in having your input for testing presence of the extensions when Jupytext is installed, do you know which folder I could test?

We do this kind of thing on some packages. For example in ipycanvas: https://github.com/conda-forge/ipycanvas-feedstock/blob/master/recipe/meta.yaml#L33-L38

This PR also commits yarn.lock which should be commited
@martinRenou martinRenou force-pushed the manifest_graft_labext_exlude_node_modules branch from 5231f22 to 476a80a Compare February 15, 2021 08:51
@martinRenou
Copy link
Contributor Author

@mwouts I've updated the PR. It is less elegant, but it seems that MANIFEST didn't like contradictory information.

@mwouts
Copy link
Owner

mwouts commented Feb 15, 2021

Thank you @martinRenou for the update! I'll give it a try very soon, tonight if I can.

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.

2 participants