Option to split install process into two steps

etamponi commented 6 years ago

In Glitch, we'd like to use pnpm as the package manager for user projects, but in order to do so, we need to split the install process into two steps:

1) the first step has to download the packages into the store (if needed) and link them to the project's node_modules directory. Additionally, we can't use hardlinks, we need to be able to specify the creation of reflinks instead. The first step should write the results needed by the second step in a file inside node_modules

2) the second step reads the file written by the first step in order to run the pre/post/install hooks for the packages that need them.

We need to split the install process because of security issues:

during the first step, we need to access the shared store. If an user project would be allowed to access the store directly, that user would be allowed to modify, remove or add packages at will, affecting every other project. Of course this is an huge security risk. The solution: run the first step (package download) outside of the user container. Moreover, we can't use hardlinks because the node_modules directory is visible and editable from inside the user container: if it contained hardlinks, an user could modify the content of widely used packages, like express. reflinks, on the other hand, are copy-on-write, so the user would never affect the shared store.
if we run the first step outside the container, we can't run install hooks, as this would be another huge security risk: through install hooks, users can run any command they want, so we can't allow install hooks outside the container boundaries.

I am already successfully running pnpm with a very ugly patch that allows me to split the install process into the two phases outlined above, so I know it works and it works well. However, maintaining a separate version of pnpm is very hard, and I would like to contribute back as much as possible. Would you be interested in integrating this two-step process in the main pnpm source, under a specific pair of options? (e.g.: first step: pnpm install --fetch-and-link-only --use-reflinks --result-file node_modules/.install-result.json; second step: pnpm install --install-hooks-only --result-file node_modules/.install-result.json)

If you're willing to, I'll write a decent PR in order for it to be included into the main supi sources.

zkochan commented 6 years ago

But this is already possible. The first part would be pnpm install --ignore-scripts and the second part would be pnpm rebuild.

Regarding reflinks, I think I don't have objections against adding an option to support them. We already support hard linking and copying

etamponi commented 6 years ago

That's... great :D I'll try it. My only question is: wouldn't pnpm rebuild also run the install scripts for the previously installed packages? E.g.: say I install socket.io, which has an install script. Then I add express. I don't want pnpm rebuild to run the install step of socket.io again.

EDIT: I just checked and indeed it would run all of the install scripts again. It would be great if I could add an --incremental option to pnpm rebuild :)

zkochan commented 6 years ago

Yes, you are right about pnpm rebuild, it will currently rebuild everything. We can make it smarter though. In fact, there is an issue about it https://github.com/pnpm/pnpm/issues/733. I think it can be even the default behavior to run the build only where it needs to be run.

etamponi commented 6 years ago

I'll look into ways of modifying rebuild then. I know that the result and installCtx objects generated by installInContext contain the information needed for deciding which packages need to run the install scripts or not. Suggestion: what if we place a special file in those packages instead of running the install hooks when ignoreScripts is set? This way, rebuild will only run the install hooks in the packages that have that file (and it would then delete the file). I guess that rebuild already contains the correct logic to iterate through the packages in the correct order.

Il 26 dic 2017 21:00, "Zoltan Kochan" notifications@github.com ha scritto:

Yes, you are right about pnpm rebuild, it will currently rebuild everything. We can make smarter though. In fact, there is an issue about it pnpm/pnpm#733 https://github.com/pnpm/pnpm/issues/733. I think it can be even the default behavior to run the build only where it needs to be run.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pnpm/supi/issues/29#issuecomment-354007384, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjUNETUPg3SddefbJQljelDRt4ZYajqks5tEVBogaJpZM4RM8QT .

etamponi commented 6 years ago

Another important question: pnpm rebuild doesn't need access to the shared store, right?

zkochan commented 6 years ago

Rebuild builds them in alphabetical order currently.

I'd prefer fewer filesystem operations. So maybe instead of files per package, one file in the root of node_modules.

pnpm rebuild doesn't need access to the store

etamponi commented 6 years ago

Then it would be exactly as I did in my patch: pnpm install first writes a node_modules/.install-result.json file, and then the second invocation of pnpm install consumes it.

If you agree, I'll do this:

pnpm install --ignore-scripts will write the install-result.json file
pnpm rebuild will use the result file if present, or keep the current behavior otherwise.

Seems reasonable?

Il 26 dic 2017 21:28, "Zoltan Kochan" notifications@github.com ha scritto:

Rebuild builds them in alphabetical order currently.

I'd prefer fewer filesystem operations. So maybe instead of files per package, one file in the root of node_modules.

pnpm rebuild doesn't need access to the store

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pnpm/supi/issues/29#issuecomment-354009957, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjUNBonaNGsR251faPn9Xw_oR9cTCg1ks5tEVb8gaJpZM4RM8QT .

zkochan commented 6 years ago

Seems fine. Also, there is already a file containing info about node_modules called node_modules/.modules.yaml, so it would make sense to save this new stuff in .modules.yaml instead of creating a new file.

etamponi commented 6 years ago

Perfect, it gets called exactly where I need it :)

I am not 100% sure about the exact stuff I need to save though. I know that result and installCtx are "enough", but they're also probably "more than enough" (sufficient but not necessary, to say it in mathematical terms). Once the PR is ready, you can probably tell me what I can remove from there :)

etamponi commented 6 years ago

https://github.com/pnpm/supi/blob/0b31c042eab1af4b5164e519a69a96cb0c1b9b5b/src/api/rebuild.ts#L126

Wouldn't this line fail if the package at hand does not come from the main registry? For example if it is a github reference?

I've the PR ready, but this might be a good time to fix this bug if it's a bug indeed.

zkochan commented 6 years ago

It will work

https://github.com/pnpm/dependency-path/blob/master/src/index.ts#L8-L17

zkochan commented 6 years ago

Published in pnpm@1.25

pnpm / supi

Option to split install process into two steps #29