Sorry, the SRPM was too large for an attachment and I can't edit the description later, so I uploaded the SRPM here:

SRPM URL: https://www.andreasgerstmayr.at/vector-2.0.0-0.1.beta.1.src.rpm

Hi Andreas, Fedora has a tool called fedora-review that downloads the latest spec and SRPM from this BZ. For this, the SPEC URL needs to be raw:
https://raw.githubusercontent.com/andreasgerstmayr/vector/rpm/vector.spec

The spec in the SRPM from Comment#1 is different to the SPEC at the above URL (they need to be identical - the tool is fussy!). Also, looks like upstream have released beta2 (and taken some or all of your stuff, which would now be in the main tarball Source0 I suppose), so could you please update to beta2, and then re-post the SPEC URL and the SRPM URL., and we'll resume the review from there.  Thanks

Hi,

We talked to the maintainers of Vector and will get to know about their release plans today.

Meanwhile I updated the spec a bit:
- as I bundle the node_modules anyway (required for running the test), I can also compile the files directly in the %build step
- added Provides: bundled(nodejs-xx) lines


Updated URLs:

SPEC URL: https://raw.githubusercontent.com/andreasgerstmayr/vector/rpm/vector.spec
SRPM URL: https://people.redhat.com/~agerstma/vector/vector-2.0.0-0.1.beta.1.src.rpm

Have started reviewing this. To start with, the %{vector_version} macro should not be necessary - instead just use %{version}. Also, the Release: line should use the dist macro, something like Release: 0.1.beta.1%{?dist}, see https://fedoraproject.org/wiki/Packaging:DistTag
Normally a snapshot release would include the git HEAD commit id, but since the upstream v2.0.0 release is imminent, we wont bother.

Also, Source1 is not a webpack, it's a tarball of (locally npm installed) node_modules, and it's huge compared to the built webpack files shipped in the binary RPM. Is there any way the tests can use a webpack too, so we could avoid bundling all of those node modules into a whopping 50MB tarball?

Also I think you should specify the following (despite this being a noarch package), as we discussed earlier :

ExclusiveArch: %{nodejs_arches}

This is because the node interpreter isn't available on some arch/dist combinations, so Fedora builds would avoid them.

I have more comments, but will post them later

Cheers

(In reply to Mark Goodwin from comment #4)
> Have started reviewing this. 

Thanks!

> To start with, the %{vector_version} macro
> should not be necessary - instead just use %{version}.
%{version} doesn't allow dashes, but the upstream version is v2.0.0-beta.1
Once there is a proper release (without dashes), I'll remove this macro.

> Also, the Release:
> line should use the dist macro, something like Release: 0.1.beta.1%{?dist},
> see https://fedoraproject.org/wiki/Packaging:DistTag
> Normally a snapshot release would include the git HEAD commit id, but since
> the upstream v2.0.0 release is imminent, we wont bother.

fixed

> Also, Source1 is not a webpack, it's a tarball of (locally npm installed)
> node_modules, and it's huge compared to the built webpack files shipped in
> the binary RPM. Is there any way the tests can use a webpack too, so we
> could avoid bundling all of those node modules into a whopping 50MB tarball?

Originally I used a real webpack (compiled JS files), but then I included the
%test step in the spec. The test runner (jest) runs on the source files and
compiles them just-in-time (see https://github.com/facebook/jest/issues/4028),
therefore it needs all dependencies.

imho the best long-term solution for this would be a moderated fedora npm registry,
as suggested by you in #1670656 ;) - but for now I think bundling the npm modules
and building & testing the package in the build step is the best solution.

We'd save a bit of space using a tar.bz2 archive (resulting size is 30MB),
but I'm not sure if this is conform to Fedora packaging guidelines (I couldn't find
any preferred/mandatory package format).

> Also I think you should specify the following (despite this being a noarch
> package), as we discussed earlier :
> 
> ExclusiveArch: %{nodejs_arches}
> 
> This is because the node interpreter isn't available on some arch/dist
> combinations, so Fedora builds would avoid them.

ok

RPM wants to create a debuginfo package now (and fails doing so), so I disabled it
for now. Should I create a dev build of vector and include it in the debuginfo
package?

> I have more comments, but will post them later

I've updated the spec and SRPM with the preliminary changes:
SPEC URL: https://raw.githubusercontent.com/andreasgerstmayr/vector/rpm/vector.spec
SRPM URL: http://people.redhat.com/~agerstma/vector/vector-2.0.0-0.1.beta.1.fc30.src.rpm


Thanks for the review, I'm looking forward to more comments.


Cheers,
Andreas

(In reply to Andreas Gerstmayr from comment #5)
[...]
> We'd save a bit of space using a tar.bz2 archive (resulting size is 30MB),
> but I'm not sure if this is conform to Fedora packaging guidelines (I
> couldn't find
> any preferred/mandatory package format).

I just checked and xz is supported. xz -9 vector_deps-2.0.0-beta.1.tar
results in an 18MB file, which is much more tolerable!

And using this as Source1 seems to work without any additional build deps:

Source1:        vector_deps-%{vector_version}.tar.xz


more comments later ..

Cheers

Perfect, I've updated the spec and SRPM (same location)

I've chatted with the upstream developers, and they'll deprecate Vector as they developed custom Grafana plugins.

I close this review request and will create a package for their new Grafana plugins once they opensourced them.