Bug 1270405

SPEC URL: https://spot.fedorapeople.org/native_client.spec
SRPM URL: https://spot.fedorapeople.org/native_client-46.0.2490.71-2.fc23.src.rpm

Does this package absolutely not work with %{ix86} for representing 32-bit? You've currently got it set for i686.

And also, shouldn't you be using our macros (like %{_prefix} for "/usr" and others) instead of hardcoding paths?

New SRPM: https://spot.fedorapeople.org/native_client-48.0.2564.82-2.fc23.src.rpm
New SPEC: https://spot.fedorapeople.org/native_client.spec

No, it doesn't currently work for i686. Nacl/PNacl is disabled for ia32. It might be possible to enable at some point, but one awful torture pit at a time. :)

As far as /usr vs %{_prefix}, if the macro is longer than the actual directory, and extremely unlikely to change in my lifetime, I'm not likely to use it religiously. That said, if it's a review blocker, I'll fix it.

https://fedoraproject.org/wiki/Packaging:NamingGuidelines says "use dashes in preference to underscores". I would suggest changing the name to "native-client".

SRPM gives 404 :(

If it is a showstopper for review, I can rename it. Upstream considers the name of this "component" to be "native_client", but I don't think they care either way.

New SRPM: https://spot.fedorapeople.org/native_client-50.0.2661.86-1.20160426gitfb00463.fc24.src.rpm
New SPEC: https://spot.fedorapeople.org/native_client.spec

Not a showstopper, dash would be just more in line with current practice.

I would really help if you extended the %description to give a two-three sentence high-level overview of what this package is. It is rather unusual package so this is particularly important in this case.

I will check the licensing later. Apart from that it's hard to do a meaningful review: basically none of the fedora guidelines are applicable.

It'd be nice to have the BR on separate lines, for the sake of sake of diffability.

ping zbyszek, do you intend on continuing this review?  you haven't commented for a couple of months.
(otherwise, I'd be happy to help pick things up)

Sorry for the delay. I'm on vacation until Monday, so I won't look at this until next week. Feel free to take it over. I'll try to finish the review next week otherwise.

It's also a terribly lousy name by virtue of being too generic.  Can't wait for "native_server".

I gave this another go today, but unfortunately mock build failed, so the review is incomplete. I'll try again in F24 mock.

Some preeliminary notes:

> BSD and GPLv3+ and GPLv3+ with exceptions and GPLv2+ with exceptions and LGPLv2+ and NCSA and MIT

Oh, my. Our guidelines say that a "breakdown" of licensing should be provided a comment in the spec file for compound licenses. There are only two items in %files:
/usr/pnacl_newlib/
/usr/pnacl_translator/
but since the build failed for me, I don't know what exactly goes in those directories.

Looking at the sources:
scons → MIT
breakpad → 3-clause BSD
native_client → 3-clause BSD
valgrind → not part of the binary package
dlmalloc → PD
newlib → 3-clause BSD
toolchain_build → various...

... so it would seem that the binary package license does not have to be so complicated (MIT and BSD would be enough?). If you could comment a bit how you arrived at the license that would help.

--

Looking at the patches, I wonder if it wouldn't be easier in the long run to keep $HOST_CLANG_PATH and to symlink $HOST_CLANG_PATH/clang → /usr/bin/gcc, $HOST_CLANG_PATH/clang++ → /usr/bin/g++…

Is the .git subdirectory needed for buildling? If not, excluding it might save quite a bit of space in the tarball. Consider adding --exclude-vcs to the tarball building commands.

--

+ package name is OK-ish (see above)
? package license is acceptable, but maybe can be simplified
+ instructions how to generate source tarballs are provided
+ Provides tags for bundled stuff are included
+ no scriptlets present or necessary
- package builds and installs OK
+ no other packaging guidelines really apply, afaict.

Created attachment 1182935 [details]
log from failed build in fedora-rawhide-x86_64 mock

(compressing to pass the maximum file size limit)

Created attachment 1183147 [details]
log from failed build in fedora-24-x86_64 mock

Seems to fail in the same way:

Running: subprocess.check_call(['/usr/bin/arm-linux-gnu-gcc', '-mcpu=cortex-a9', '-D__arm_nonsfi_linux__', '-I/usr/arm-linux-gnu/include/', '-O2', '-Wall', '-Werror', '-I../../../..', '-DNACL_LINUX=1', '-DDEFINE_MAIN', '-c', '../../../src/nonsfi/irt/irt_interfaces.c', '-o', '../unsandboxed_runtime_arm_linux_install/translator/arm-linux/lib/unsandboxed_irt.o'], cwd='/builddir/build/BUILD/native_client/toolchain_build/out/unsandboxed_runtime_arm_linux_work')
../../../src/nonsfi/irt/irt_interfaces.c:7:20: fatal error: assert.h: No such file or directory
 #include <assert.h>
                    ^
compilation terminated.
Error building unsandboxed_runtime_arm_linux: Command '['/usr/bin/arm-linux-gnu-gcc', '-mcpu=cortex-a9', '-D__arm_nonsfi_linux__', '-I/usr/arm-linux-gnu/include/', '-O2', '-Wall', '-Werror', '-I../../../..', '-DNACL_LINUX=1', '-DDEFINE_MAIN', '-c', '../../../src/nonsfi/irt/irt_interfaces.c', '-o', '../unsandboxed_runtime_arm_linux_install/translator/arm-linux/lib/unsandboxed_irt.o']' returned non-zero exit status 1

I could call it chromium-native_client. Is that name preferred?

As to the license, toolchain_build is where all the fun happens. We're talking about binutils, gcc, newlib, llvm (and components).

Also, the tooling in native_client depends on the .git files (I think. This code build process is incredibly fragile and poorly understood by humans.)

(In reply to Tom "spot" Callaway from comment #13)
> I could call it chromium-native_client. Is that name preferred?

Works for me, assuming this is only relevant for chromium.

Okay. I _think_ things are working again.

New SRPM: https://spot.fedorapeople.org/chromium-native_client-52.0.2743.82-1.20160725git7d72623.fc24.src.rpm
New SPEC: https://spot.fedorapeople.org/chromium-native_client.spec

I've confirmed that with this chromium-native_client (and some tweaks I just committed to chromium), nacl/pnacl support works again. I would appreciate assistance in getting this last beastly piece added to Fedora.

Please consider renaming chromium-native_client → chromium-native-client. The mix of dash and underscore is ugly (and current policy is to prefer dashes).

rpmlint complains that the last changelog entry has no version specified.

It builds fine now. No files outside of /usr/pnacl_{translator,newlib}, so this package should not break anything in the distro. OK, I'll get out of the way: package is approved.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/chromium-native_client

chromium-52.0.2743.82-9.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-ccb3e3d8e1

chromium-native_client-52.0.2743.82-1.20160725git7d72623.fc24 chromium-52.0.2743.82-9.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-dc48263ff8

chromium-52.0.2743.82-9.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-ccb3e3d8e1

chromium-52.0.2743.82-9.fc24, chromium-native_client-52.0.2743.82-1.20160725git7d72623.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-dc48263ff8

chromium-52.0.2743.82-9.fc24, chromium-native_client-52.0.2743.82-1.20160725git7d72623.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

chromium-52.0.2743.82-9.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.