[rush] Copy over package binaries during deploy

[willsoto]

I’ve implemented most of what was recommended in the issue, but I’m not sure where to write the tests.

I also haven’t implemented this for the script linking. I want to make sure I’m headed in the right direction.

Closes #1982

[ghost]

All CLA requirements met.

[octogonz]

I added a couple comments, but otherwise this is on the right track. 👍

[willsoto]

Awesome. I’ll try and wrap this up tomorrow then. Thanks for the feedback.

[willsoto]

@octogonz one question I do have is how to link the bins in create-links.js. I noticed this comment at the top:

rushstack/apps/rush-lib/src/scripts/create-links.ts

Lines 11 to 12 in 89076ae

	// API borrowed from @rushstack/node-core-library, since this script avoids using any
	// NPM dependencies.

In the default mode I am using @pnpm/link-bins, so is the expectation I do the same here? Do I inline the work with credit to the package? Other suggestions?

[octogonz]

@octogonz one question I do have is how to link the bins in create-links.js. I noticed this comment at the top:

rushstack/apps/rush-lib/src/scripts/create-links.ts

Lines 11 to 12 in 89076ae

	// API borrowed from @rushstack/node-core-library, since this script avoids using any
	// NPM dependencies.

In the default mode I am using @pnpm/link-bins, so is the expectation I do the same here? Do I inline the work with credit to the package? Other suggestions?

Hmm.. That is a hitch that I had not considered. Inlining probably won't work because that package has numerous dependencies. Maybe we need to copy+transform the PNPM output here.

Did you investigate step 5 from #1982 (comment) yet? That seems related.

[willsoto]

@octogonz one question I do have is how to link the bins in create-links.js. I noticed this comment at the top:

rushstack/apps/rush-lib/src/scripts/create-links.ts

Lines 11 to 12 in 89076ae

	// API borrowed from @rushstack/node-core-library, since this script avoids using any
	// NPM dependencies.

In the default mode I am using @pnpm/link-bins, so is the expectation I do the same here? Do I inline the work with credit to the package? Other suggestions?

Hmm.. That is a hitch that I had not considered. Inlining probably won't work because that package has numerous dependencies. Maybe we need to copy+transform the PNPM output here.

Did you investigate step 5 from #1982 (comment) yet? That seems related.

I haven't investigated step 5 yet, I was about to when I noticed that comment. I'll test it out

[octogonz]

BTW if it turns out we need to change our approach, and that the PR so far is sufficient to solve your own case, I would be okay with merging+publishing a partial solution in the interim.

[willsoto]

@octogonz this works for my use case in docker. Only default strategy is supported for the reason above.

I also created a changelog here. I did it cause CI was failing on that task. If you prefer to create the entry yourself, please feel free.

[octogonz]

this works for my use case in docker. Only default strategy is supported for the reason above.

@willsoto To clarify, you're done working on this PR for now, and it is ready for final reviewing+merging. The cases we didn't handle should occur in a later PR. Is that right?

[willsoto]

@octogonz that is correct. I am not sure how to resolve the "no external dependencies" problem for the script at the moment

[octogonz]

I am not sure how to resolve the "no external dependencies" problem for the script at the moment

I'm thinking it will maybe require a fundamentally different approach. Either:

1. Make our own lightweight reimplementation of the @pnpm/link-bins algorithm; OR
2. Read the files that PNPM installed in apps\api-extractor\node_modules\.bin\*, apply some RegExp's to transform their contents, and write the result into deploy\apps\api-extractor\node_modules\.bin\*. (See #5 in the design note: [rush] rush deploy does not copy over package binaries #1982 (comment) )

I don't want to block this PR behind that, however.

[willsoto]

I am not sure how to resolve the "no external dependencies" problem for the script at the moment

I'm thinking it will maybe require a fundamentally different approach. Either:

1. Make our own lightweight reimplementation of the `@pnpm/link-bins` algorithm; OR

2. Read the files that PNPM installed in `apps\api-extractor\node_modules\.bin\*`, apply some RegExp's to transform their contents, and write the result into `deploy\apps\api-extractor\node_modules\.bin\*`.

I don't want to block this PR behind that, however.

Agreed. This PR is done from my perspective then unless I forgot something.

edit: just rebased and resolved conflicts

[octogonz]

I fixed a couple minor issues. Otherwise LGTM

[octogonz]

This fix was released with Rush 5.29.0

[ujwal-setlur]

@willsoto @octogonz, this does not work when linkCreation: "script". It works when linkCreation: "default". Is that expected? Do I need to edit deploy-metadata.json manually before creating links?

[octogonz]

this does not work when linkCreation: "script". It works when linkCreation: "default". Is that expected? Do I need to edit deploy-metadata.json manually before creating links?

@ujwal-setlur looks like this problem is now tracked by #2846

[ujwal-setlur]

@octogonz thank you!