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

MAINT Improvements to node and file_packager invocations #1600

Merged
merged 10 commits into from
May 29, 2021

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented May 22, 2021

This PR does two things:

  1. I moved package.json into the root directory. Why? We are using npx from the Makefile which is executing usually under the root directory. If we install node_modules in src/js then these invocations don't see node_modules and install an extra copy of the packages. So we want node_modules in the root directory. node wants to put node_modules in the same directory as package.json hence I moved package.json. I also updated the build rules so that any recipe that uses npx has a dependency on node_modules/.installed.
  2. We invoke file_packager.py from both the Makefile (in two places) and from build_pkg. There has been some drift in which options are applied in the two places. I extracted the common options and the task of locating the file_packager.py script into tools/file_packager.sh. I also shortened the make rules for distutils.data and test.data and made some minor updates to the dev docs about file_packager.py.

@rth
Copy link
Member

rth commented May 28, 2021

+1 for the standardizing the file_packager.py arguments. I had similar thoughts a few weeks, more along the lines of using a python function in both case but this also works.

We are using npx from the Makefile which is executing usually under the root directory. If we install node_modules in src/js then these invocations don't see node_modules and install an extra copy of the packages.

I don't think that's an ideal solution.

  • First because tomorrow maybe we will also run npx from some other subfolder and we don't want node_modules in each such subfolder (unless I'm missing something about the spirit of npm packaging)
  • second if we are to create a npm package, I think it would be much better have a specific folder and say this is that package (which would have package.json) in its root. Rather than package.json at the root of pyodide. For the same reason that we don't want the setup.py from pyodide-build in the root folder.

Wouldn't setting NODE_PATH work? It doesn't look like npx have a --prefix option npm/npx#74 ? And if there is no solution, than maybe we should revisit the use npx for instance in favor of npm exec?

@hoodmane
Copy link
Member Author

tomorrow maybe we will also run npx from some other subfolder and we don't want node_modules in each such subfolder

It works in any folder below the folder with the node_modules.

@hoodmane
Copy link
Member Author

hoodmane commented May 28, 2021

But I agree a different solution may well be preferable.

@hoodmane
Copy link
Member Author

According to my tests, setting NODE_PATH doesn't work.

@rth
Copy link
Member

rth commented May 28, 2021

It works in any folder below the folder with the node_modules.

I could live with a symlink of ./src/js/node_modules to ./node_modules, done in the Makefile, if there is no other way and it doesn't break things. It's not ideal though, and it feels like there should be some better solution.

BTW, it might be good to upload what we have now to NPM and mark it as alpha or similar; that would also constrain how we can organize files to some extent.

@hoodmane
Copy link
Member Author

it feels like there should be some better solution.

Yeah I was a bit disappointed that node doesn't have a solution to this. I think they are really assuming that package.json sits in the root directory of the project.

It is possible to give full paths like ./src/js/node_modules/rollup/dist/bin/rollup, but npx automates the task of determining where to find the right object to execute inside of node_modules/rollup.

@hoodmane
Copy link
Member Author

Okay I'm going to merge this, if we find some better way of dealing with node_modules in the future we can remove the symlink.

@hoodmane hoodmane merged commit 2e04752 into pyodide:main May 29, 2021
@hoodmane hoodmane deleted the simpler-file-packager-invocations branch May 29, 2021 17:31
hamlet4401 pushed a commit to tytgatlieven/pyodide that referenced this pull request Jul 3, 2021
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