#3 Rename nodejs-yarn binary package to yarnpkg (similar to other distros) and consistently use macros in spec
Merged a month ago by zvetlik. Opened 2 months ago by ngompa.
rpms/ ngompa/nodejs-yarn rename-binary-package-to-yarnpkg  into  master

file modified
+27 -12

@@ -8,13 +8,13 @@ 

  %global enable_tests 1

  

  # don't require bundled modules

- %global __requires_exclude_from ^%{_prefix}/lib/node_modules/yarn/.*$

+ %global __requires_exclude_from ^%{nodejs_sitelib}/yarn/.*$

  

  Name:           nodejs-yarn

  Version:        1.13.0

- Release:        3%{?dist}

+ Release:        4%{?dist}

  Summary:        Fast, reliable, and secure dependency management.

- Url:            https://github.com/yarnpkg/yarn

+ URL:            https://github.com/yarnpkg/yarn

  # we need tarball with node_modules

  Source0:        %{npm_name}-v%{version}-bundled.tar.gz

  Source1:        yarn-tarball.sh

@@ -27,11 +27,21 @@ 

  

  BuildRequires:  nodejs-packaging

  BuildRequires:  npm

- Requires:       nodejs

  

  %description

  Fast, reliable, and secure dependency management.

  

+ %package -n yarnpkg

+ Summary:        Fast, reliable, and secure dependency manager for JavaScript

+ Requires:       nodejs

+ # Package was renamed when Fedora 32 was rawhide

+ # Don't remove this before Fedora 34

+ Obsoletes:      %{name} < 1.13.0-4

+ Provides:       %{name} = %{version}-%{release}

+ 

+ %description -n yarnpkg

+ Yarn is a fast, reliable, and secure dependency manager for JavaScript.

+ 

  %prep

  %setup -q -n %{npm_name}-%{version}

  

@@ -51,8 +61,8 @@ 

      %{buildroot}%{nodejs_sitelib}/%{npm_name}

  

  mkdir -p %{buildroot}%{_bindir}

- ln -sf ../lib/node_modules/yarn/bin/yarn.js %{buildroot}%{_bindir}/nodejs-yarn

- ln -sf ../lib/node_modules/yarn/bin/yarn.js %{buildroot}%{_bindir}/yarnpkg

+ ln -sfr %{buildroot}%{nodejs_sitelib}/%{npm_name}/bin/yarn.js %{buildroot}%{_bindir}/yarnpkg

+ ln -sfr %{buildroot}%{nodejs_sitelib}/%{npm_name}/bin/yarn.js %{buildroot}%{_bindir}/%{fc_name}

  

  # Remove executable bits from bundled dependency tests

  find %{buildroot}%{nodejs_sitelib}/%{npm_name}/node_modules \

@@ -64,19 +74,24 @@ 

  %if 0%{?enable_tests}

  %check

  %nodejs_symlink_deps --check

- if [[ $(%{buildroot}/%{_bindir}/nodejs-yarn --version) == %{version} ]] ; then echo PASS; else echo FAIL; fi

- if [[ $(%{buildroot}/%{_bindir}/yarnpkg --version) == %{version} ]] ; then echo PASS; else echo FAIL; fi

+ if [[ $(%{buildroot}%{_bindir}/yarnpkg --version) == %{version} ]] ; then echo PASS; else echo FAIL && exit 1; fi

+ if [[ $(%{buildroot}%{_bindir}/%{fc_name} --version) == %{version} ]] ; then echo PASS; else echo FAIL && exit 1; fi

  %endif

  

  

- %files

- %{nodejs_sitelib}/yarn

- %{_bindir}/nodejs-yarn

- %{_bindir}/yarnpkg

+ %files -n yarnpkg

  %doc README.md README.Fedora.md

  %license LICENSE

+ %{_bindir}/yarnpkg

+ %{_bindir}/%{fc_name}

+ %{nodejs_sitelib}/%{npm_name}

  

  %changelog

+ * Thu Dec 05 2019 Neal Gompa <ngompa@datto.com> - 1.13.0-4

+ - Rename nodejs-yarn binary package to yarnpkg (similar to other distros)

+ - Use nodejs macros consistently throughout spec

+ - Make the tests fail the build if the tests fail

+ 

  * Thu Jul 25 2019 Fedora Release Engineering <releng@fedoraproject.org> - 1.13.0-3

  - Rebuilt for https://fedoraproject.org/wiki/Fedora_31_Mass_Rebuild

  

This PR renames the binary package to yarnpkg to be consistent with other distributions (e.g. Debian/Ubuntu).

Moreover, I've included some small fixes to consistently use macros throughout the spec.

Surely this should be done properly via a rename review, not by hacking the spec to generate a binary package with a different name to the source package...

@zvetlik @tomh Please take a look as soon as possible.

I'd appreciate a speedy review, merge, and push into Fedora releases.

Yes that sort of attitude is definitely the way to get people on side. As far as I know I'm not even a maintainer on this!

Also no way should this be done in existing released versions - if it happens it should be rawhide only.

@tomh No, there's plenty of precedent for doing it this way. Most recently, we did it with python-black.

The source package is still definitely nodejs-yarn, and if there were more things for it to provide, that would make sense to do too.

@tomh Why would it be a problem in stable releases? Package renames are perfectly safe, given the obsoletes and such. No functionality is changing otherwise.

I'm sorry if it sounded like I was being rude. I'm doing some internal work for $DAYJOB that involves Yarn and I figured that the improvements I made would be useful to upstream to Fedora for everyone else to benefit from. I assumed you were a co-maintainer given that the Nodejs SIG has commit access to it.

@tomh Now I get why you think I was being rude. Sorry, your message didn't show up while I was typing my comment asking for the review.

rebased onto d90ab7b

2 months ago

As someone who's had to use yarnpkg in the past, I'm in favor of this change as long as it meets the proper obsoletes, provides stuff. My look at the changes shows that it does, but I'm not an obsoletes/provides expert.

When I was using it in the past, any my project needed "yarnpkg". I had to first find the right package, then convince myself it was the correct one by looking at the rpm source and npm repository.

Look I don't dispute that this is probably a good idea, and it was probably a mistake not to name it that originally.

I also don't dispute that what is proposed here will work in a technical sense, and I appreciate that other people may have done it. I just don't think it's appropriate to do it this way - it creates a needless distinction between the source and binary rpm names and it seems the only reason is to avoid the need for a rename review and allow it to be done more quickly. Well rename reviews exist for a reason and if we accept this sort of thing then who would ever bother with one when they can just do this?

Equally I accept that a rename is "safe" in an existing release. I do however question whether it is in line with an updates policy whose basic philosophy for stable releases is "we should avoid major updates of packages within a stable release" but I guess that depends on whether you view it as major - in some ways it is trivial but it other ways it appears to be a significant change.

In any case this is @zvetlik's package so I'll butt out now and leave the decision to her ;-)

To find a middle-ground, could we just add Provides: yarnpkg = ENVR for now in stable and Rawhide (so dnf install yarnpkg will work) and then do the proper rename (with review) in Rawhide?

rebased onto 8522ecf

2 months ago

LGTM, although it's a shame this did not get pointed out during package review. I'd also be in favour of renaming it in stable releases, but maybe this could be discussed on the mailing list if someone has strong objections.

Sorry for taking so long to look at this.

Pull-Request has been merged by zvetlik

a month ago