Bug 1270405 - Review Request: chromium-native_client - Google Native Client Toolchain
Review Request: chromium-native_client - Google Native Client Toolchain
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Zbigniew Jędrzejewski-Szmek
Fedora Extras Quality Assurance
:
Depends On: 1270355 1270357 1270358 1270364 1270368 1270375 1312963
Blocks: 1270322
  Show dependency treegraph
 
Reported: 2015-10-09 19:28 EDT by Tom "spot" Callaway
Modified: 2016-08-08 19:53 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-08-01 12:23:39 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zbyszek: fedora‑review+


Attachments (Terms of Use)
log from failed build in fedora-rawhide-x86_64 mock (551.43 KB, text/plain)
2016-07-22 15:21 EDT, Zbigniew Jędrzejewski-Szmek
no flags Details
log from failed build in fedora-24-x86_64 mock (552.00 KB, application/x-xz)
2016-07-23 15:27 EDT, Zbigniew Jędrzejewski-Szmek
no flags Details

  None (edit)
Description Tom "spot" Callaway 2015-10-09 19:28:05 EDT
Spec URL: https://spot.fedorapeople.org/native_client.spec
SRPM URL: https://spot.fedorapeople.org/native_client-45.0.2454.101-2.fc23.src.rpm
Description: 
Google's "pnacl" toolchain for native client support in Chromium. Depends on
their older "nacl" toolchain, packaged separately.
Fedora Account System Username: spot

This package is pretty huge and disgusting, but it should be compliant. I could not unbundle or split this out, because of how Google's custom build framework works. This package is needed to build native client support in Chromium.
Comment 2 Neal Gompa 2016-01-26 01:53:24 EST
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?
Comment 3 Tom "spot" Callaway 2016-02-29 11:40:09 EST
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.
Comment 4 Zbigniew Jędrzejewski-Szmek 2016-04-12 22:08:47 EDT
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 :(
Comment 5 Tom "spot" Callaway 2016-04-27 13:22:56 EDT
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
Comment 6 Zbigniew Jędrzejewski-Szmek 2016-05-01 23:31:29 EDT
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.
Comment 7 Rex Dieter 2016-07-08 10:17:23 EDT
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)
Comment 8 Zbigniew Jędrzejewski-Szmek 2016-07-08 10:26:52 EDT
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.
Comment 9 Orion Poplawski 2016-07-19 13:45:52 EDT
It's also a terribly lousy name by virtue of being too generic.  Can't wait for "native_server".
Comment 10 Zbigniew Jędrzejewski-Szmek 2016-07-22 15:18:31 EDT
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.
Comment 11 Zbigniew Jędrzejewski-Szmek 2016-07-22 15:21 EDT
Created attachment 1182935 [details]
log from failed build in fedora-rawhide-x86_64 mock

(compressing to pass the maximum file size limit)
Comment 12 Zbigniew Jędrzejewski-Szmek 2016-07-23 15:27 EDT
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
Comment 13 Tom "spot" Callaway 2016-07-25 14:10:22 EDT
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.)
Comment 14 Orion Poplawski 2016-07-25 18:48:54 EDT
(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.
Comment 16 Tom "spot" Callaway 2016-07-27 11:29:32 EDT
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.
Comment 17 Zbigniew Jędrzejewski-Szmek 2016-07-27 11:44:41 EDT
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.
Comment 18 Gwyn Ciesla 2016-07-27 11:52:37 EDT
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/chromium-native_client
Comment 19 Fedora Update System 2016-07-28 17:01:53 EDT
chromium-52.0.2743.82-9.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-ccb3e3d8e1
Comment 20 Fedora Update System 2016-07-28 17:02:15 EDT
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
Comment 21 Fedora Update System 2016-07-30 15:27:30 EDT
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
Comment 22 Fedora Update System 2016-07-30 15:27:55 EDT
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
Comment 23 Fedora Update System 2016-08-01 12:23:31 EDT
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.
Comment 24 Fedora Update System 2016-08-08 19:53:03 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.