Skip to content

[rush] When creating symbolic links, use relative paths #805

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

Merged
merged 3 commits into from
Sep 28, 2018

Conversation

octogonz
Copy link
Collaborator

No description provided.

@octogonz
Copy link
Collaborator Author

@jbcpollak could you try this branch and see whether it fixes your Docker issue?

@octogonz
Copy link
Collaborator Author

octogonz commented Sep 8, 2018

@jbcpollak did you ever have a chance to look at this?

@jbcpollak
Copy link

@pgonzal - sorry for the delay - the shifting priorities of a startup - I'll take a look ASAP, still very much on my radar

@jbcpollak
Copy link

@pgonzal - how do I try a branch locally?

@octogonz
Copy link
Collaborator Author

You run node rush-lib/lib/start.js.

I have a batch file RushTest.cmd in my Windows shell PATH like this:

@ECHO OFF
@SETLOCAL
node D:\GitRepos\wbt\apps\rush-lib\lib\start.js %*

This allows me to run "rushtest" instead of "rush" for the purpose of testing.

@jbcpollak
Copy link

@pgonzal this seems to work. I had to downgrade my node version which caused problems with my binary compiled extensions, even though I did update -p --full, but everything appears to resolve correctly.

I should be able to use this to build docker containers, I'll just need to change my base-path in my Dockerfile.

Any chance this could be merged soon?

@jbcpollak
Copy link

BTW, our plan with this is to add the following to our Dockerfile:

COPY ../../../common/temp/node_modules /common/temp/node_modules

the downside to this is that every docker container we build for each project in the mono repo will have the complete setup of all dependencies for all project in the repo, which is pretty ugly. I can't think of a solution for that.

@octogonz
Copy link
Collaborator Author

@jbcpollak which package manager are you using? With PNPM there might be a way to optimize this.

@jbcpollak
Copy link

jbcpollak commented Sep 22, 2018 via email

@octogonz
Copy link
Collaborator Author

BTW, our plan with this is to add the following to our Dockerfile:

COPY ../../../common/temp/node_modules /common/temp/node_modules

the downside to this is that every docker container we build for each project in the mono repo will have the complete setup of all dependencies for all project in the repo, which is pretty ugly. I can't think of a solution for that.

I see, so these are NodeJs server projects that cannot just be webpacked into a deployable bundle. And they get developed/compiled in a monorepo, but when you build the container that you will ship, they need to bring along all their dependencies.

If you were to run pnpm install in that folder would it work? Or would it fail to find the locally linked Rush projects (because they don't really get published to an NPM registry)? That problem might be solvable by running pnpm pack as part of your build, which makes an NPM tarball. And then you could use a pnpmfile.js to tell pnpm install where to find the tarballs.

I feel like pnpm install is probably the right tool if the problem is to pick out a minimal subset of dependencies for a project and arrange them into a node_modules folder.

@zkochan and @iclanton who would be interested in this

@jbcpollak
Copy link

jbcpollak commented Sep 22, 2018

Would pnpm install work with rush? I considered that - the way pnpm works, it creates hidden hardlinks in your projects node_modules folder back to the pnpm store, then symlinks to the correct hardlinks. This would work for docker because the symlinks would refer to the hardlinks in the docker container, and the hardlinks are actually the files they reference, so it should just work. Each project would only have the hardlinks it needs.

The reason it falls apart with rush is that pnpm install is only run on the temp project in common/temp, and then rush handles the symlinks for each project. Because of this, the dependencies refer back to common/temp/node_modules, which has ALL the hardlinks for everything in monorepo.

I considered trying a manual pnpm install step in each project, but I don't see how that would correctly resolve links between library projects within the monorepo and the server executables. Plus,I don't think I can reuse the rush magic lock file (in common/temp) so dependencies might fluctuate.

What we currently do at deployment time is run the following steps:

rm -rf node_modules
npm install // note that is npm, not pnpm
npm clean; npm build
deploy.sh // script that does the dockerization and publishing of servers, or package and publish of libraries

This relies on the fast that rush will run the deploy command in dependency order, so libraries will be published to our npm repository, then re-fetched from there by server projects.

This is obviously really bad because we use pnpm at development and test time, then npm at deployment time. We could switch to pnpm and reference the rush store, and that would probably fix the problem. The reason I've hesitated to do that is that this second run of pnpm install would not use the lock file in common/temp, so the dependency versions might vary between test and deployment. This is happening now with the npm install since it obviously doesn't read the pnpm lock file either,and it's causing variation in what is tested and released. Plus this whole process is slow.

It's probably a lot of work, but if rush ran pnpm install in each project and setup the linking between projects natively, that would solve this problem. I'm not sure if that is a problem for other reasons though

@octogonz
Copy link
Collaborator Author

How many projects are in your repo, and how many docker containers do you make? (I assume each docker container is based on one project folder that acts as the entry point / application.)

@octogonz
Copy link
Collaborator Author

(My thought being, maybe we can promote your use case into a first class feature of Rush, rather than just a symlink fix that enables a workaround. It certainly fits the charter of what Rush is all about.)

@jbcpollak
Copy link

We've got at least six projects that get built to docker containers, and 3 or more shared libraries. The frustrating thing is we've got different services using different frameworks, all which have their own dependency trees, so it's silly to build a bunch of docker containers that all have 3 different frameworks worth of dependencies in them.

First class support would be great!

@octogonz
Copy link
Collaborator Author

octogonz commented Sep 23, 2018

@jbcpollak Would it be difficult to create a small GitHub repo that illustrates how you make the docker container by copying from the common/temp folder? I'd be interested to try out the tarball idea I suggested above, but I don't have much experience with Docker.

@iclanton
Copy link
Member

iclanton commented Sep 28, 2018

Approved.

Approved with PullApprove

@octogonz octogonz merged commit c35d780 into master Sep 28, 2018
@octogonz
Copy link
Collaborator Author

Any chance this could be merged soon?

@jbcpollak this is in now. It will be published tomorrow with Rush 5.3.0

@jbcpollak
Copy link

jbcpollak commented Sep 29, 2018 via email

@octogonz octogonz deleted the pgonzal/rush-relative-symlinks branch October 3, 2018 23:37
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.

4 participants