nodejs-packagiing improvements

[T.C. Hollingsworth]

I've pushed a few new nodejs-packaging features to an "experimental"
branch in the git repository:
https://git.fedorahosted.org/cgit/nodejs-packaging.git/tree/?h=experimental

The changes include:

- %nodejs_fixdep now supports a --dev argument to affect devDependencies

This is used by the jquery review I recently filed to drop
dependencies on the nonfree jshint library.

- %nodejs_fixdep also supports an --optional argument to affect
optionalDependencies

I added this for completeness' sake.  We currently don't do anything
with optionalDependencies.  If the plans for soft dependencies come to
fruition and they support automatic population like normal Requires
then we'll probably start doing something with these, but for now we
ignore them.

- %nodejs_symlink_deps now supports a --force argument

This argument overrides failing the build when bundled dependencies
are encountered in node_modules.  This can be used during development
to allow you to mix system-provided and npm-provided modules.  It
should _never_ be used in an official Fedora package.

- %nodejs_symlink_deps now supports --build as an alias for --check

This switch more accurately reflects what it does (i.e. it works in
the build directory not the buildroot directory) and doesn't look so
weird in %build sections.  ;-)

And finally, the biggie:

- Support for the caret (^) operator was added to the dependency generator.

This has been supported in npm for about a year now, but the
dependency generator never knew about it.  It also hasn't really
gained traction in the wild till very recently. (old npm versions that
don't support it are slowly dying out...)

The semantics of this new operator are described in detail here:
https://github.com/isaacs/node-semver#ranges

Basically, ^1.2.3 is the equivalent of ">1.2.3 <2".  This is much
better for our purposes than the "~1.2.3" form used by many packages,
which translates to ">1.2.3 <1.3".  Undoubtedly many of us will want
to start using it in %nodejs_fixdep for certain recalcitrant packages.
 ;-)

Testing builds of the new nodejs-packaging are available from this copr:
copr.fedoraproject.org/coprs/patches/js-future/

Please try it out and let me know if there's anything broken, any more
improvements you've just been dying to have, etc.

Thanks!
-T.C.

[T.C. Hollingsworth]

On Wed, Mar 19, 2014 at 10:19 AM, T.C. Hollingsworth
<tchollingsworth(a)gmail.com&gt; wrote:
>  - Support for the caret (^) operator was added to the dependency
> generator.
>
>  This has been supported in npm for about a year now, but the
>  dependency generator never knew about it.  It also hasn't really
>  gained traction in the wild till very recently. (old npm versions that
>  don't support it are slowly dying out...)
>
>  The semantics of this new operator are described in detail here:
>  https://github.com/isaacs/node-semver#ranges
>
>  Basically, ^1.2.3 is the equivalent of ">1.2.3 <2".  This is much
>  better for our purposes than the "~1.2.3" form used by many packages,
>  which translates to ">1.2.3 <1.3".  Undoubtedly many of us will want
>  to start using it in %nodejs_fixdep for certain recalcitrant packages.
>   ;-) 
Thinking about this further, perhaps we should add a --caretify option
or so to %nodejs_fixdep.

This option would change deps thusly (in npm-speak and rpm-speak, respectively):
-convert '1.2.3' deps to '^1.2.3'
    changes '==1.2.3' to '>1.2.3 <2'
-convert '~1.2.3' deps to '^1.2.3'
    changes '>=1.2.3 <1.3' to '>1.2.3 <2'
-convert '1.2.x' deps to '^1.2'
    changes '>=1.2 <1.3' to '>=1.2 <2'
-convert '1.2' deps to '^1.2'
    changes '>=1.2 <1.3' to '>=1.2 <2'
-skip '1' deps, since '^1' would be functionally equivalent
    both translate to '>=1 <2'
-skip when one of '><=' are present, since this option makes no sense
with such deps

This option would permit no packages to be passed as arguments (e.g.
just '%nodejs_fixdep --caretify'), in which case it would affect all
dependencies for which it makes sense and skip those for which it does
not.

It would also permit a single package as an argument (e.g.
'%nodejs_fixdep --caretify async'), in which case it would only affect
that package, and error out instead of skipping packages when it is
not possible.  (e.g. it will fail the build if you try to caretify a
dependency with a '><=' in it, which is rare, but will still skip a
plain major version dep like '1' since the dependency will be
effectively equivalent if we appended a ^.)

I think this approach to fixing deps will be much nicer in a lot of
cases than what we're currently doing.  If we manually '%nodejs_fixdep
async ^1.2.3' all the time, we'll probably forget to check if the
lower bound gets higher on us, and potentially end up with
combinations of packages that don't work together.  This option would
eliminate manually specifying the version in the spec, so the lower
bound is always taken from package.json, we just adjust the upper
bound to something that makes a lot more sense for Fedora (and the
general ecosystem).

Make sense?  Would other packagers find this useful?

-T.C.

[Jamie Nguyen]

On 21/03/14 00:54, T.C. Hollingsworth wrote:
>  Thinking about this further, perhaps we should add a --caretify
> option
>  or so to %nodejs_fixdep.
>  
>  This option would change deps thusly (in npm-speak and rpm-speak, respectively):
>  -convert '1.2.3' deps to '^1.2.3'
>      changes '==1.2.3' to '>1.2.3 <2'
>  -convert '~1.2.3' deps to '^1.2.3'
>      changes '>=1.2.3 <1.3' to '>1.2.3 <2'
>  -convert '1.2.x' deps to '^1.2'
>      changes '>=1.2 <1.3' to '>=1.2 <2'
>  -convert '1.2' deps to '^1.2'
>      changes '>=1.2 <1.3' to '>=1.2 <2'
>  -skip '1' deps, since '^1' would be functionally equivalent
>      both translate to '>=1 <2'
>  -skip when one of '><=' are present, since this option makes no sense
>  with such deps
>  
>  This option would permit no packages to be passed as arguments (e.g.
>  just '%nodejs_fixdep --caretify'), in which case it would affect all
>  dependencies for which it makes sense and skip those for which it does
>  not.
>  
>  It would also permit a single package as an argument (e.g.
>  '%nodejs_fixdep --caretify async'), in which case it would only affect
>  that package, and error out instead of skipping packages when it is
>  not possible.  (e.g. it will fail the build if you try to caretify a
>  dependency with a '><=' in it, which is rare, but will still skip a
>  plain major version dep like '1' since the dependency will be
>  effectively equivalent if we appended a ^.)
>  
>  I think this approach to fixing deps will be much nicer in a lot of
>  cases than what we're currently doing.  If we manually '%nodejs_fixdep
>  async ^1.2.3' all the time, we'll probably forget to check if the
>  lower bound gets higher on us, and potentially end up with
>  combinations of packages that don't work together.  This option would
>  eliminate manually specifying the version in the spec, so the lower
>  bound is always taken from package.json, we just adjust the upper
>  bound to something that makes a lot more sense for Fedora (and the
>  general ecosystem).
>  
>  Make sense?  Would other packagers find this useful? 
I'm not sure I'd use or promote such a feature. Currently, we are using
one or more lines of "%nodejs_fixdep" in %prep and it is very clear to
see exactly what decisions the packager has made, and how it affects the
dependencies. With just `--caretify async`, you can't tell what version
the package depends on without cross-referencing the Spec with the
package.json.

Any interested user that isn't familiar with our Node packaging macros
may want to take a look at the Spec. They can already guess what
%nodejs_fixdep async '^1.2.3' will do, but are much less likely to guess
what %nodejs_fixdep --caretify will do.

It makes sense when updating a Node package for the packager to review
the %nodejs_fixdep lines and adjust as necessary. I always diff the
package.json from the old tarball to the new tarball to see exactly what
has changed. Adding options as above might make life easier for the
packager, but may also breed carelessness, especially when using the
blanket `--caretify` for all dependencies.

As long as "%nodejs_fixdep" gains support for caret notation, I think
that's enough.


Kind regards,

-- 
Jamie Nguyen

[T.C. Hollingsworth]

On Fri, Mar 21, 2014 at 10:59 AM, Jamie Nguyen <j(a)jamielinux.com&gt; wrote:
>  I'm not sure I'd use or promote such a feature. 
And I don't blame you given I did such a poor job at putting forward
my proposal.  :-)  I'm singing the benefits of the new dependency
style without explaining what's so much better about them.  Let me try
again...

At the moment, most node modules use the tilde form of expressing
dependencies.  This means they use something like ~1.2.3, which
translates to >1.2.3 <1.3.  This shorthand was stolen from rubygems.

As it turns out, this highly restrictive form of dependency has been
criticized in both the ruby and nodejs communities as being too
restrictive.  Espeically in the nodejs community it's unwarranted; the
semantic versioning standard [1] that node modules are supposed to
follow (and many do in practice) dictates that the major version must
be bumped when backwards incompatible changes are made to a library.
Thus, regardless of what the dependency says it's generally possible
to upgrade to a new minor version of a library while keeping its
dependents working.

A year ago, npm upstream introduced the caret (^) operator as an
alternative to the tilde operator.  With this operator, something like
^1.2.3 translates to >1.2.3 <2.  This form of dependency much more
accurately reflects what will actually work, and is more aligned with
typical Fedora practice, where we often want to use newer compatible
versions of libraries, First being one of our foundations and all.

In the last month, npm upstream recently switched 'npm install --save'
to use carets instead of tildes, given that a year has passed since
support for it was introduced so most npm installations can reasonably
be expected to support it.  Unfortunately, as the recent attempts to
kill off the unfortunate use of a self-signed certificate for
registry.npmjs.org indicate, there are still a lot of old npm
installations in the wild (mostly thanks to PaaS providers), so many
module authors are going to choose not to use the caret form to retain
compatibility with these ancient npm versions for some time to come.

Of course, in Fedora we control the npm version provided, and indeed
we've had an npm version that supports this (even if the dependency
generator does not) for a year now as well.  So we're free to
encourage the use of it whenever we want, and using them in place of a
tilde likely reflects what upstream would love to do if they didn't
have to support PaaS providers that move at glacial paces.

Let's step back and take a look at what we're doing now.  We often
want to introduce newer versions of libraries to the package
collection than are typically defined in dependents' package.json
files.  When we do that, we have to bump the dependency in all
dependent packages.  So if we want to update nodejs-foomodule from
1.2.3 to 1.3.4, we often have to go add '%nodejs_fixdep ~1.3.4' to all
dependent packages.  For certain core modules used by a lot of
dependents, this could be a lot of work for what should be a simple
upgrade.

If we instead used something like '%nodejs_fixdep ^1.2.3' in those
dependent packages, we eliminate the need to push updates that consist
solely of modifying package.json to potentially dozens of dependent
modules whenever we update a heavily-depended-upon module.  We can
instead just update it and everything will Just Work.  Much nicer,
IMHO.

But doing this with classic %nodejs_fixdep is tedious.  Now instead of
just updating a handful of %nodejs_fixdep calls in %prep, you'd have
to double-check every one against the new package.json and make sure
you update the lower bound if it moved up.

Enter '%nodejs_fixdep --caretify'. Instead of manually keeping up with
the tilde->caret conversion, it's just done for you automatically at
build time.  It's explicitly saying "don't allow dependencies that are
of a lower version than is defined in package.json, but allow later
versions up to but not including the next major version".  It does
what's right for Fedora without second-guessing upstream too much,
unlike our current policy which overrides upstream to a greater
degree.

I should note that in practical terms, 'sed -i s/~/^/g package.json'
would accomplish the same thing, but by adding a special option to
%nodejs_fixdep we can introduce error-checking that makes packagers
less likely to shoot themselves in the foot.  (I'm not entirely happy
with --caretify as the argument, I'm open to better suggestions.)

I don't think it reduces the ability of packagers to know what's going
on in a package.  With our current practice, you either need to look
at the combination of the package.json pre-%prep and the spec file to
know how they're going to turn out, or else just look at package.json
post-%prep.  With my proposal, the vast majority of packages will just
use '--tildefy' and nothing else, so you can just look at package.json
pre-%prep and know all those tildes are going to be converted to
carets later on, or look at package.json post-%prep as per usual.

That being said, a possible alternative would be to just pretend
tildes are actually carets in the dependency generator.  Then no spec
changes are required ever, but package.json won't accurately reflect
what could be possibly installed on the system.  I rather like that
package.json and RPM are in sync at all times when it comes to modules
in the package collection.  Explicit is better than implicit.  ;-)

tl;dr
- tilde dependencies are evil
- we have something better now with carets
- we really should use them in Fedora, it'll make our lives so much easier ;-)
- given the above, --tildefy makes sense and eliminates tedious,
unnecessary manual labor on our parts

I really think there's no point in us playing dependency cops to the
degree we have been with current practices, and adopting the above
will allow us to bring much more awesomeness to the distribution much
more quickly without sacrificing quality in any way.

-T.C.

[1] http://semver.org/

[T.C. Hollingsworth]

On Fri, Mar 21, 2014 at 10:59 AM, Jamie Nguyen <j(a)jamielinux.com&gt; wrote:
>  As long as "%nodejs_fixdep" gains support for caret
> notation, I think
>  that's enough. 
FWIW, %nodejs_fixdep does no error checking of any sort and will write
whatever you pass to it in package.json without question, which means
it already supports carets.

I figured duplicating the error checking here was pointless.  The
dependency generator will already fail hard later on in the build,
regardless of whether you do something wrong with %nodejs_fixdep or
upstream messed it up.

-T.C.

[Jamie Nguyen]

On 19/03/14 17:19, T.C. Hollingsworth wrote:
>  - %nodejs_fixdep now supports a --dev argument to affect
> devDependencies 
Great.


>  - %nodejs_symlink_deps now supports --build as an alias for --check
>  
>  This switch more accurately reflects what it does (i.e. it works in
>  the build directory not the buildroot directory) and doesn't look so
>  weird in %build sections.  ;-) 
That does make much more sense.


>  And finally, the biggie:
>  
>  - Support for the caret (^) operator was added to the dependency generator. 
Sweet. Came across this in nodejs-grunt-contrib-uglify and had to
%nodejs_fixdep the dependency:
https://bugzilla.redhat.com/show_bug.cgi?id=977125#c7


Kind regards,

-- 
Jamie Nguyen