+1

Can we say executables, applications or tools instead of binaries? They are not really binary.

IIRC @sgallagh made it so you would BR nodejs-devel, but I'm not sure.

This is a rather big chunk of copy-pasta. Shouldn't we try to macronize it?

Why this conditional? If so, can we at least use a bcond?

I'd prefer if this script had an upstream. Maintaining code on a committee-only-approved docs website is... hard. I suggest https://pagure.io/nodejs-packaging

Can we say executables, applications or tools instead of binaries? They are not really binary.

Good point. I will change it to "applications"

IIRC @sgallagh made it so you would BR nodejs-devel, but I'm not sure.

nodejs-devel does require nodejs-packaging and so BR nodejs-devel would work. But it would also pull in nodejs. This is for if something needs just the nodejs-macros.

I had thought that nodejs-macros was a requirement of redhat-rpm-config but found that it currently is not. That's why I added this section. If we get redhat-rpm-config updated, I'll take this part out.

This is a rather big chunk of copy-pasta. Shouldn't we try to macronize it?

Yes, and we will if this is passed.
We didn't want to do the work of macronizing it if it didn't get approved.

Why this conditional? If so, can we at least use a bcond?

Many / most of the nodejs spec files have this conditional. But this is a because of the problems we were having with the nodejs libraries. You are correct, this conditional shouldn't be needed and I will remove it.

I'd prefer if this script had an upstream. Maintaining code on a committee-only-approved docs website is... hard. I suggest https://pagure.io/nodejs-packaging

I completely agree. I couldn't figure out where to put it. I will talk with other nodejs-packaging maintainers and if they agree, put it there, with a link to it here.

nodejs-devel does require nodejs-packaging and so BR nodejs-devel would work. But it would also pull in nodejs. This is for if something needs just the nodejs-macros.

IMHO Bringing in nodejs to build a nodejs package should be mandatory. It is not such a big time saving if you don't. And when you do, you can assume nodejs is always available when building nodejs packages, which is useful. That's why all Python packages MST BR python3-devel (even when building with plain python3 would do).

I had thought that nodejs-macros was a requirement of redhat-rpm-config but found that it currently is not. That's why I added this section. If we get redhat-rpm-config updated, I'll take this part out.

IIRC we decided not to do that, because it brings in Python.

nodejs-devel does require nodejs-packaging and so BR nodejs-devel would work. But it would also pull in nodejs. This is for if something needs just the nodejs-macros.

IMHO Bringing in nodejs to build a nodejs package should be mandatory. It is not such a big time saving if you don't. And when you do, you can assume nodejs is always available when building nodejs packages, which is useful. That's why all Python packages MST BR python3-devel (even when building with plain python3 would do).

I had thought that nodejs-macros was a requirement of redhat-rpm-config but found that it currently is not. That's why I added this section. If we get redhat-rpm-config updated, I'll take this part out.

IIRC we decided not to do that, because it brings in Python.

Ah, that's right. I'd forgoten why it wasn't there. I'm fine changing this to nodejs-devel, which makes the next section somewhat redundant.
I'll make that change, and do a bit of a re-write on this part.

Despite the name of that last commit I believe I have covered everything.
* I changed the word binaries to applications
* I changed BuildRequires: nodejs-packaging to nodejs-devel and changed corresponding wording around it.
* I removed the %if statement from the %checking section
* I removed the in-document bundling script, put that script along with some documentation over in the nodejs-packaging repo, and updated the section, along with a link to the nodejs-packaging repo.

A nit pick: Should this be in %prep instead?

Also this.

My only remaining concern is the big amount of copypasta. I'll take your word on macronizing it in the next step. Let me know if you want help with the design or even code of the macros.

+1 in general (possibly a FPC member might want to convert this to sembr and check the formatting, but I won't volunteer for that)

Finally, I'd like to check if the snippets work. Do you have a working spec file following this? Could you please share? Also consider adding a full spec example to the guidelines.

A nit pick: Should this be in %prep instead?

Ya, I agree. When I was re-working this I was trying to figure out why I had it in %build, but not in %prep.
I'll move it to %prep, and I think when we generate the macros, maybe we can make a generic nodejs setup macro.

Also this.

No, the untarring of the dev tarball needs to be in %check and not %prep or %build.
It is very possible that the %install section will pull in some of the dev bundling stuff. Although that's not the end of the world, usually the dev/testing bundle is much bigger.

It is very possible that the %install section will pull in some of the dev bundling stuff.

Let me make sure I understand this correctly: If the dev tarball is unpacked before running whatever we run in %install, the extra dev requirements would get installed together with the bundled runtime deps? If that's the case I recommend explicitly saying this in the guidelines, otherwise packagers might get the same idea as me ("let's move thsi to %prep"). What about:

Note that the tarball with the dev dependencies needs to be unpacked in %check and not in %prep to avoid accidentally bundling the unpackaged dependencies that are only needed for testing.

My only remaining concern is the big amount of copypasta. I'll take your word on macronizing it in the next step. Let me know if you want help with the design or even code of the macros.

+1 in general (possibly a FPC member might want to convert this to sembr and check the formatting, but I won't volunteer for that)

Finally, I'd like to check if the snippets work. Do you have a working spec file following this? Could you please share? Also consider adding a full spec example to the guidelines.

I hadn't thought of that, putting a real spec file example in.
I've got close to a whole spec file as an example in https://src.fedoraproject.org/rpms/nodejs-packaging. But you are right, sometimes a real-world, kick the tires example is better than a general one.

I'll get you an example package that you can look at.

It is very possible that the %install section will pull in some of the dev bundling stuff.

Let me make sure I understand this correctly: If the dev tarball is unpacked before running whatever we run in %install, the extra dev requirements would get installed together with the bundled runtime deps? If that's the case I recommend explicitly saying this in the guidelines, otherwise packagers might get the same idea as me ("let's move thsi to %prep"). What about:

Note that the tarball with the dev dependencies needs to be unpacked in %check and not in %prep to avoid accidentally bundling the unpackaged dependencies that are only needed for testing.

Good point, sometimes when you write everything you assume people know what's in your head. I will put that in.

I shifted the setup of the prod bundled tarball to %prep.
I added a note, and note in the example comments, that the dev bundled tarball needs to be in %check.
I added a full example spec file. With this specfile, and the output of the bundling script "nodejs-packaging-bundler tape" you can successfully build an install-able package locally, or on koji.

%%prep

Suggestion: %{npm_name} instead of type here? Also couple lines above.

%%prep

Fixed in next commit

Suggestion: %{npm_name} instead of type here? Also couple lines above.

Good catch. I was inconsistent.
Fixed in next commit.

%%prep and inconsistent %{npm_name} fixed.

One general observation I might make is that it would be much better if the newly added prose avoided long lines and used semantic line breaks instead. I know the original text doesn't use them (because it was never fixed after the wiki autoconversion) but converting any changed text doesn't really make the diff much more difficult to view.

OK, FPC has voted to accept this and fix up a couple of things later (sembreaks and pulling out the spec template into the examples directory). I'll merge this now and fix those up afterwards.

As tibbs said, we disccussed this today (here: https://meetbot-raw.fedoraproject.org/fedora-meeting-1/2021-01-14/fpc.2021-01-14-17.02.txt):

• PR#1034 - Change the prefered method of nodejs packaging to bundle
  nodejs libraries. (geppetto, 17:13:01)
• LINK: https://pagure.io/packaging-committee/pull-request/1034
  (geppetto, 17:13:18)
• ACTION: Change the prefered method of nodejs packaging to bundle
  nodejs libraries (+1:5, 0:0, -1:0) (geppetto, 17:27:33)