#10 Fix path regex in nodejs.attr
Closed by jstanek. Opened by jstanek.
jstanek/nodejs-packaging master  into  master

Download 10.patch

The old regex had too many \s to correctly match the module path.

Minimal reproducer:

# Find location of npm on Fedora 29
$ find /usr/lib /usr/lib64 -path '*/npm/package.json'
/usr/lib/node_modules/npm/package.json
# Try to match the path using the old regex
$ find /usr/lib /usr/lib64 -path '*/npm/package.json' | grep -E '^/usr/lib(64)?/node_modules/[^/]+/package\\.json$' || echo 'NO MATCH'
NO MATCH

After the change:

$ find /usr/lib /usr/lib64 -path '*/npm/package.json' | grep -E '^/usr/lib(64)?/node_modules/[^/]+/package\.json$' || echo 'NO MATCH'
/usr/lib/node_modules/npm/package.json

That's a pretty extraordinary claim which, if true, would mean that automatic dependency generation wasn't being done, which clearly isn't true.

So there must be more to this story than the above description suggests!

As an example, here's one built a week ago where the dependency generators clearly ran:

https://koji.fedoraproject.org/koji/rpminfo?rpmID=15500030

Do you have an example of a more recent one where they failed? Has something maybe changed in how rpm parses the attrs file?

The issue was noticed on npm rpm:

https://koji.fedoraproject.org/koji/rpminfo?fileStart=200&rpmID=15404597&fileOrder=name&buildrootOrder=-id&buildrootStart=0#filelist
https://koji.fedoraproject.org/koji/rpminfo?fileStart=200&rpmID=15404720&fileOrder=name&buildrootOrder=-id&buildrootStart=0#filelist

The npm bundles lots of modules (/usr/lib/node_modules/npm/node_modules), and should thus have Provides: bundled(nodejs-X) = VER for each of them -- which it does not.

When I fed the path to the npm's package.json to the provide script, it correctly outputted them:

# in nodejs-packaging root; using head for brevity
$ echo /usr/lib/node_modules/npm/package.json | ./nodejs.prov | head
bundled(nodejs-JSONStream) = 1.3.4
bundled(nodejs-abbrev) = 1.1.1
bundled(nodejs-agent-base) = 4.2.0
bundled(nodejs-agentkeepalive) = 3.4.1
bundled(nodejs-ajv) = 5.5.2
bundled(nodejs-ansi-align) = 2.0.0
bundled(nodejs-ansi-regex) = 2.1.1
bundled(nodejs-ansi-regex) = 3.0.0
bundled(nodejs-ansi-styles) = 3.2.1
bundled(nodejs-ansicolors) = 0.3.2

Then I started to play with the regex in nodejs.attr with results described in the first comment, and so arrived at the conclusion that the regex is at fault.

Granted, I might be overlooking something, and I'm not sure how to test the generator properly…

Hmm. Well npm is a bit of a special case but I can't see any obvious reason for that.

It's not at all clear that double backslash is correct mind, and other .attr files are inconsistent - some use a single backslash in this case and some use two and my guess is that both "work" for whatever reason when used the way rpm uses them.

Ha. The nodejs spec doesn't BR nodejs-packaging so the dependency generator is not present when npm is created.

Yes, I found out that now as well, currently rebuilding node with added
BuildRequires: nodejs-packaging to see if it helps.

Note that it introduces a circular build dependency for node on itself,
since nodejs-packaging require node(engine) -- is that intentional?

Well I imagine that's just to ensure you have nodejs installed without having to require it separately.

I was going to ask @sgallagh if there was a good reason why nodejs didn't BR nodejs-packaging before doing anything as drastic as trying to rebuild it ;-)

@tomh The only major issue I see is the bootstrapping case: if the nodejs package BuildRequires: nodejs(engine), then it will cause issues for bringing up a new architecture.

I'd recommend that if we add this requirement, it should be in the non-bootstrap case (there is already a conditional for this in the spec).

@sgallagh well, here it is a transitive dependency (nodejsnodejs-packagingnode(engine)).

The question is does nodejs-packaging need the node(engine)? From glancing over the sources and specfile, it seems that nothing really uses it, and the require is there for historic reasons. Of course, if node packages expect that the nodejs-packaging will also bring node(engine), the bootstrapping option might be for the best.

@jstanek if you remove it you will break every leaf module that tries to run tests as they won't have the node interpreter - they will be relying on nodejs-packaging cause the interpreter to be installed.

@tomh I feared so. In that case, since my rebuild of nodejs with the BR included did indeed produce the bundled provides, let me cancel this PR and open a different one against nodejs itself.

In https://fedoraproject.org/wiki/Packaging:Node.js?rd=Node.js/#BuildRequires implies heavily that this is all you need to BR to build a node package, so I don't think we can drop the Requires: nodejs(engine) there without a lot of noise and breakage.

Pull-Request has been closed by jstanek

Metadata