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

Change DEB nodejs relation from Depends to Recommends #916

Merged
merged 2 commits into from
Oct 14, 2016

Conversation

pesho
Copy link
Contributor

@pesho pesho commented Oct 12, 2016

Summary

A "Recommends" relation preserves the out-of-the-box experience for most people, as recommended packages are installed by default on modern Debian / Ubuntu distros. But it allows people who install Node.js using a different method to opt out of the nodejs DEB package.

Test plan

$ bash scripts/build-deb.sh
...
$ sudo dpkg -i artifacts/yarn_0.15.1_all.deb

^ the above commands execute successfully.

$ which yarn 
/usr/bin/yarn
$ yarn version
yarn version v0.15.1
info Current version: 0.15.1

A "Recommends" relation preserves the out-of-the-box experience for most people,
as recommended packages are installed by default on modern Debian / Ubuntu
distros. But it allows people who install Node.js in a different manner to opt
out of the nodejs DEB package.
@Daniel15
Copy link
Member

Daniel15 commented Oct 12, 2016

I was worried about the out-of-the-box experience. Ideally we don't want people that use Yarn to even have to know what Node.js is, it should "just work". If Debian and Ubuntu automatically install recommended dependencies then I think this is fine.

One issue I can think of is with Ubuntu 14.04. The Node.js version in its repository is too old (0.10.x), so installation of Yarn fails until you add a repository that has a newer version of Node.js (such as NodeSource's). With the change in this PR, I'm worried that Yarn will install with no problems, as the Node.js requirement is no longer a hard dependency. Is that what will happen?

recommended packages are installed by default on modern Debian / Ubuntu distros

Does Ubuntu 14.04 do this too? It's old but there's still quite a few people using it.

Could you please test it on a fresh Ubuntu VM or VPS that does not have Node.js installed and verify that it installs Node.js? You can clone our Debian repo from https://github.com/yarnpkg/releases, add your package with reprepro -b debian includedeb stable /path/to/your-file.deb, and test with a local URL (although you'll need to replace the GPG key in debian/conf/distributions with one of your own in order to build a local repo)

But it allows people who install Node.js using a different method to opt out of the nodejs DEB package.

How do you opt-out from installing the recommended packages?

@pesho
Copy link
Contributor Author

pesho commented Oct 12, 2016

How do you opt-out from installing the recommended packages?

By passing the --no-install-recommends option to apt-get.

Does Ubuntu 14.04 do this too? It's old but there's still quite a few people using it.

Could you please test it on a fresh Ubuntu VM or VPS that does not have Node.js installed and verify that it installs Node.js? You can clone our Debian repo from https://github.com/yarnpkg/releases, add your package with reprepro -b debian includedeb stable /path/to/your-file.deb, and test with a local URL (although you'll need to replace the GPG key in debian/conf/distributions with one of your own in order to build a local repo)

Yes, it is true on Ubuntu 14.04 as well. Here is an easier way to test it: Compare the output of these two commands:

docker run --rm -it ubuntu:14.04 bash -c "apt-get update && apt-get install -y mc"

and

docker run --rm -it ubuntu:14.04 bash -c "apt-get update && apt-get install -y --no-install-recommends mc"

One issue I can think of is with Ubuntu 14.04. The Node.js version in its repository is too old (0.10.x), so installation of Yarn fails until you add a repository that has a newer version of Node.js (such as NodeSource's). With the change in this PR, I'm worried that Yarn will install with no problems, as the Node.js requirement is no longer a hard dependency. Is that what will happen?

You are right, I think. But this should be solvable by other means. Here are two options that come to mind:

  • Add a Conflicts relationship with old versions of the nodejs package.
  • Add a runtime check against process.versions.node.

@pesho
Copy link
Contributor Author

pesho commented Oct 12, 2016

(ref nodejs/docker-node#243 for posterity)

@Daniel15
Copy link
Member

Thank you for the information! I'll do some more testing across a few different Ubuntu/Debian versions and see how it goes.

Should we also do this change for the CentOS RPM? It also has a dependency on nodejs: https://github.com/yarnpkg/yarn/blob/master/scripts/build-deb.sh#L64

@pesho
Copy link
Contributor Author

pesho commented Oct 12, 2016

Should we also do this change for the CentOS RPM? It also has a dependency on nodejs: https://github.com/yarnpkg/yarn/blob/master/scripts/build-deb.sh#L64

Not sure really, I don't have much experience with RPM. If its notion of Recommends works similar to how it does in DEBs, and if CentOS/RHEL/Fedora also install recommended packages by default, then probably yes.

I'll do some more testing across a few different Ubuntu/Debian versions and see how it goes.

Thanks!

@Daniel15
Copy link
Member

Daniel15 commented Oct 13, 2016

So I did some testing with this change. I have it sitting on my test server if you'd like to try it out

deb https://yarndev.dan.cx/debian/ stable main

The out-of-the-box experience with Ubuntu 14.04 is not ideal, due to the fact that Node.js 4.0+ isn't available in the repo. It shows that nodejs is recommended, but doesn't automatically install it (since the version doesn't match). The current package would totally fail at this point:

It's fine once you add the NodeSource Node 6.x repo:

Ubuntu 16.04 is also fine out-of-the-box:

Do you think that's a reasonable tradeoff? I guess we could make yarn a shell script that checks whether Node.js is installed, and show a nicer error message if not (recommending to use the NodeSource repo or nvm). That defeats some of the purposes of using packages (maintaining all your apps using them) but it'd allow nvm.

@emilv
Copy link

emilv commented Oct 13, 2016

Can I run Yarn without having nodejs? If not, then nodejs is a dependency for Yarn. I just checked how other Debian packages are doing it, such as node-ws and npm. They all list nodejs in Depends.

If you install Nodejs in some other way than through Apt you essentially tell the system that you don't want their dependency-resolving package manager. Faking the dependencies of the Yarn to circumvent that is not the right way to go.

If you installed Nodejs by some other means than through Apt, I suggest that you create a meta-package to tell that to Apt. Check out equivs which was built to do just that. Another solution is to create a real nodejs package using checkinstall, which also gives you the side benefit of being easy to uninstall.

@eknkc
Copy link
Contributor

eknkc commented Oct 13, 2016

@emilv those packages (npm, node-ws) are 3 years old now. not the best role models to work with.

I don't think a single person installs npm using apt from distro repos. node itself does not recommend these ancient packages.

yarn deb package should provide a convenient way to install it. Telling people to create meta packages would not serve that purpose.

@emilv
Copy link

emilv commented Oct 13, 2016

They are perfect role models for how a package made for the Apt package manager are supposed to be. Wouldn't it be crazy if an Npm package didn't use semantic versioning? This is no different. It's crazy to not specify a hard dependency in the Depends field.

@eknkc
Copy link
Contributor

eknkc commented Oct 13, 2016

@emilv I think I could not express myself properly.

The npm package on debian repo was last updated at 2014. Both on stable and testing. That's the reason I'm dismissing them, they are abandoned.

nodejs on debian stable is 0.10.29 which is not really a dependency of yarn because it's not capable of running yarn. Having a hard dependency on nodejs does not make any more sense than depending on ffmpeg in that case.

@pesho
Copy link
Contributor Author

pesho commented Oct 14, 2016

The current package would totally fail at this point:

@Daniel15 well it would refuse to install, which is not necessarily a total failure in this case :)

Do you think that's a reasonable tradeoff?

My preference is slightly towards yes, but I admit it might be a regression in certain scenarios. If it leads to converting Yarn into a shell script in order to have a nicer error message when a compatible Node can't be auto-installed, then it's probably not worth the trouble.

But on second thought, it's not very likely that someone would attempt to install Yarn without having Node already installed somehow.

A slightly better variation of the current PR would be replacing the current

Recommends: nodejs (>= 4.0.0)

with

Recommends: nodejs
Conflicts: nodejs (< 4.0.0)

This would have the advantage of refusing to install on Ubuntu 14.04 if only the distro-provided nodejs package is installed. I can update this PR if you think it's preferable to the current state.

@Daniel15
Copy link
Member

it's not very likely that someone would attempt to install Yarn without having Node already installed somehow.

Well, Yarn is aiming to be reasonably language-agnostic (notice the home page doesn't mention JavaScript at all), so eventually we'll have users that have never used Node.js before (and some that don't really care about Node.js, much like most of us don't really care about glibc or the MSVC runtime or any other runtime dependencies of apps we use every day.

This would have the advantage of refusing to install on Ubuntu 14.04 if only the distro-provided nodejs package is installed.

It'll still install with no errors if they don't have Node.js installed at all, but I guess that's fine. Add the Conflicts line and I'll merge this 😄

@pesho
Copy link
Contributor Author

pesho commented Oct 14, 2016

@Daniel15 done, PR updated. Tested the result on Ubuntu 14.04 and 16.04 and it works as expected.

@Daniel15
Copy link
Member

Thank you! This helped me learn a bit more about dpkg 😄

@Daniel15 Daniel15 merged commit 12a60ca into yarnpkg:master Oct 14, 2016
@Daniel15
Copy link
Member

FYI, I had to change < 4.0.0 to << 4.0.0 as it was throwing an obsolete-relation-form lint warning.

@pesho
Copy link
Contributor Author

pesho commented Oct 15, 2016

Thanks. I somehow missed that (as Lintian is also throwing other warnings).

@Daniel15
Copy link
Member

Lintian should only have two other errors right now, both due to third-party code:

W: yarn: package-contains-vcs-control-file usr/share/yarn/node_modules/acorn/.gitattributes
W: yarn: non-standard-executable-perm usr/share/yarn/node_modules/object-path/LICENSE 0744 != 0755

There's quite a few other warnings that are ignored though, but they shouldn't appear when you run lintian.

Daniel15 pushed a commit that referenced this pull request Oct 16, 2016
* Change DEB nodejs relation from Depends to Recommends

A "Recommends" relation preserves the out-of-the-box experience for most people,
as recommended packages are installed by default on modern Debian / Ubuntu
distros. But it allows people who install Node.js in a different manner to opt
out of the nodejs DEB package.

* Add "Conflicts" relationship with DEB nodejs < 4.0.0
Daniel15 added a commit to yarnpkg/release-infra that referenced this pull request Oct 16, 2016
@Daniel15
Copy link
Member

Pushed to the package repository as version 0.15.1-2. Thanks for your contribution!

@Daniel15
Copy link
Member

@eknkc - Just noticed this comment:

The npm package on debian repo was last updated at 2014. Both on stable and testing.

Ubuntu has a newer version, 3.5.2: http://packages.ubuntu.com/xenial/npm. This also depends on nodejs rather than just recommending or suggesting it.

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.

None yet

4 participants