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

feat: copy LICENSE file from monorepo root on pack #6366

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tien
Copy link

@tien tien commented Jun 30, 2024

What's the problem this PR addresses?

#6359

How did you fix it?

Add workspace root LICENSE via genPackList & genPackStream.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@tien
Copy link
Author

tien commented Jul 17, 2024

Hey @merceyz @arcanis, sorry for the ping. I believe this feature requires a breaking change. It's not super crucial, just a feature of pnpm that would be nice to have in Yarn. So I'm happy to keep this PR open and wait for the next available major release slot if you guys think that's the best way to go 🙏.

@tien
Copy link
Author

tien commented Jul 25, 2024

Friendly 🏓 @merceyz, @arcanis

const files = paths.map(path => ({file: path, source: ppath.resolve(workspace.cwd, path)}));

if (workspace.cwd !== project.cwd) {
const workspaceHasLicense = paths.some(path => path.match(LICENSE_PATTERN));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoding something about LICENSE files in this function feels a little too specific. Perhaps instead the files set via the files parameter should all be case insensitive?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @arcanis the regex is case-insensitive. Also, in this case, it's a little hard to reduce specificity because what we are doing is so specific :p. Would love any suggestion for alternatives.

Comment on lines +73 to +74
const normalizedFiles = files?.map(file => typeof file === `string`
? {file, source: ppath.resolve(workspace.cwd, file)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to make source a string parameter (and to read the content of the license file outside of the genPackStream function); this way it gives us a way to generate files within the archive that don't necessarily exist on disk (for example if we wanted to generate the LICENSE file from the SPDX code).

Copy link
Author

@tien tien Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe source is already is a string because PortablePath is in fact a string. I'm not too aware of the differences between the 2 though.

Also because I'm not too familiar with how gzip and tar works. Can give me further guidance on where the inclusion of LICENSE files should happen instead? Are you suggesting that genPackStream should also accept something like { file: PortablePath, content: string | Buffer }?

@tien tien requested a review from arcanis July 31, 2024 23:35
@tien
Copy link
Author

tien commented Sep 6, 2024

@arcanis any updates regarding the feedbacks? 🙏

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.

3 participants