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

Update: use a named version folder in NPM tarballs #4094

Merged
merged 9 commits into from
Aug 8, 2017
Merged

Conversation

BYK
Copy link
Member

@BYK BYK commented Aug 4, 2017

Summary

Fixes #3758. Makes the top-level folder in the tar archives have a name like yarn-vX.Y.Z instead of dist using the --transform and -s options in tar (they are different in GNU and BSD tar).

Test plan

Run yarn build-dist and then tar -ztvf artifacts/yarn-v1.0.0.tar.gz. Make sure the output lists all the files under yarn-v1.0.0 directory.

@BYK BYK requested a review from Daniel15 August 4, 2017 11:46
@BYK
Copy link
Member Author

BYK commented Aug 4, 2017

@Daniel15 - I'm now worried that this may break deb packages so any guidance around that would be appreciated.

@Daniel15
Copy link
Member

Daniel15 commented Aug 6, 2017

Looks good to me! I was going to say that we need to update the installation script, but we were clever and added --strip 1 to the tar extract command so it's just stripping off the first piece of the path and thus doesn't care what it is.

deb looks fine, you can check the "artifacts" tab of CircleCI: https://circleci.com/gh/yarnpkg/yarn/4701#artifacts/containers/0. The file name has the correct version, and checking the package with dpkg-deb (ie. curl https://4701-49970642-gh.circle-artifacts.com/0/home/ubuntu/yarn/artifacts/yarn_1.0.0_all.deb | dpkg-deb -I -) shows the right version number on the package itself.

However, the Windows build is broken, as it's looking for a dist directory:

<!-- Ensure Yarn build was performed already -->
<Error
Condition="!Exists('$(YarnDistPath)\bin\yarn.js')"
Text="Please run scripts\build-dist.sh before building the installer"
/>

build-npm.sh (which pushes Yarn to npm) also looks for a dist directory:

version=`./dist/bin/yarn --version`

Maybe the easiest solution here would be to rename the directory when building the tarball, then rename it back? Then all the scripts in Yarn itself could keep on using dist.

Copy link
Member

@Daniel15 Daniel15 left a comment

Choose a reason for hiding this comment

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

Windows build is broke

@Daniel15
Copy link
Member

Daniel15 commented Aug 6, 2017

Oh, even better @BYK: GNU tar has a --transform option to transform the file names when archiving: https://stackoverflow.com/a/9729944/210370. We could do --transform s/^dist/yarn-vx.x.x/ when archiving to change the directory name when creating the archive, without actually physically renaming it. Note that Mac OS comes with BSD tar rather than GNU tar but you can use Homebrew to get the GNU version.

@BYK
Copy link
Member Author

BYK commented Aug 7, 2017

Note that Mac OS comes with BSD tar rather than GNU tar but you can use Homebrew to get the GNU version.

Ah, this is very unfortunate. I want this code to work across platforms without issues and looks like GNU tar also has an -s option with a completely different meaning. I don't want to branch code by trying to guess which tar version people has so I think I'll go ahead and fix other places where dist is used. It would have been great of we could simply use this option tho :(

An alternative would be to just have a symlink named dist to the new, named folder but I guess that'd break on Windows then.

@BYK
Copy link
Member Author

BYK commented Aug 7, 2017

I stand corrected: we need that dist folder so I'm gonna bite the bullet and detect the type of tar and tailor the command to it. Also leave the dist folder intact for the Windows installer.

@BYK
Copy link
Member Author

BYK commented Aug 7, 2017

@Daniel15 ready

Copy link
Member

@Daniel15 Daniel15 left a comment

Choose a reason for hiding this comment

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

This looks good. The other alternative would be to rename the dist dir, then tar it, then rename it back, but this looks cleaner IMO. It sucks that BSD tar and GNU tar differ here, but oh well, at least we can detect the version. That's a pretty clever approach.

@@ -13,7 +13,7 @@ ensureAvailable lintian
ensureAvailable rpmbuild

PACKAGE_TMPDIR=tmp/debian_pkg
VERSION=`dist/bin/yarn --version`
VERSION=`./artifacts/yarn-legacy-* --version`
Copy link
Member

Choose a reason for hiding this comment

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

We can probably revert this back, right?

Copy link
Member Author

@BYK BYK Aug 8, 2017

Choose a reason for hiding this comment

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

Yeah, we can. I'd leave this one though since this script doesn't rely on dist being there, but the other one does so I'll revert that one before landing.

@@ -3,7 +3,7 @@

set -ex

version=`./dist/bin/yarn --version`
version=`./artifacts/yarn-legacy-* --version`
Copy link
Member

Choose a reason for hiding this comment

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

This too?

@BYK BYK merged commit 5cfb241 into master Aug 8, 2017
@BYK BYK deleted the dist-artifacts branch August 8, 2017 09:32
GulajavaMinistudio added a commit to GulajavaMinistudio/yarn that referenced this pull request Aug 9, 2017
Update: use a named version folder in NPM tarballs (yarnpkg#4094)
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