Bug 1270357 - Review Request: nacl-gcc - Various compilers (C, C++) for nacl
Summary: Review Request: nacl-gcc - Various compilers (C, C++) for nacl
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1270355
Blocks: 1270322 1270358 1270405
TreeView+ depends on / blocked
 
Reported: 2015-10-09 18:39 UTC by Tom "spot" Callaway
Modified: 2016-04-26 21:21 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-04-15 21:33:38 UTC
zbyszek: fedora-review+


Attachments (Terms of Use)

Description Tom "spot" Callaway 2015-10-09 18:39:40 UTC
Spec URL: https://spot.fedorapeople.org/nacl-gcc.spec
SRPM URL: https://spot.fedorapeople.org/nacl-gcc-4.4.3-15.gitf80d6b9.fc23.src.rpm
Description: 
The gcc package contains the GNU Compiler Collection version 4.4.3.
You'll need this package in order to compile C code. This provides support
for nacl targets.
Fedora Account System Username: spot

This is the gcc target for the nacl x86_64 toolchain needed for Chromium. It has to be built twice, once as a bootstrap, then once after nacl-newlib is built.

Comment 1 Richard Shaw 2016-03-16 18:58:33 UTC
Quick spec review while at $DAYJOB...

May not matter but I use this with one of my packages so I can paste in the full commit but use the short one in the resultant archive.

%global commit caad49c0945065f4b7bfa3ccb96523e4766a9727
%global shortcommit %(c=%{commit}; echo ${c:0:7})

As much as I dislike it, and I do, unless the package guidelines have changed don't we need the checkout date in front of the "git" part of the release in YYYYMMDD format?

Are you intentionally supporting f21 and below? (and later f19?) I think the conditionals can be dropped at this point.

Comment 2 Zbigniew Jędrzejewski-Szmek 2016-03-17 03:00:54 UTC
Some more macros would simplify things a bit:
cd  obj-%{gcc_target_platform}
make DESTDIR=%{buildroot} install ...
↓
%make_install -C obj-%{gcc_target_platform}

%autosetup -p1

%license for COPYING*.

Why not parallelized make? E.g. %make_build ?

Summary should be more concrete: C and C++ compilers for nacl
(unless there's something more, but I don't think).

I think the Summary should also have some small explanation what nacl is (just in case somebody is looking for a normal compiler and they see this package).

The %description seems outdated: "The gcc package contains the GNU Compiler Collection version 4.4.3. You'll need this package in order to compile C code."

No %check? There seems to be many test cases... I don't know how feasible it would be to run them.

This all looks very reasonable. Apart from some cleanups I don't see anything to change.

Comment 3 Tom "spot" Callaway 2016-03-23 20:25:02 UTC
I'm not a fan of the "super macros" you've suggested, with the exception of %license. I'd rather know exactly what's going on there and be able to tweak it. Also, autosetup doesn't allow for the use of patch specific suffixes, which makes it very easy to rediff patches when upstream changes.

As to %check, I'm not sure that Google has bothered to add nacl specific checks. They seem to only be supporting nacl in a legacy state at this point, despite pNacl depending on it.

New SPEC: https://spot.fedorapeople.org/nacl-gcc.spec
New SRPM: https://spot.fedorapeople.org/nacl-gcc-4.4.3-16.20150504gitf80d6b9.fc24.src.rpm

- fix versioning
- remove ancient conditionals
- fix ExclusiveArch to be only for x86_64
- use license tag for COPYING files
- improve summary and description
- add smp_mflags to make invocations in build

Comment 4 Zbigniew Jędrzejewski-Szmek 2016-03-24 03:58:58 UTC
rmplint | grep -v devel-file-in-non-devel-package:

nacl-gcc.src:61: W: configure-without-libdir-spec
nacl-gcc.src:125: E: hardcoded-library-path in %{_prefix}/lib/gcc/%{gcc_target_platform}/
nacl-gcc.src: W: invalid-url Source0: nacl-gcc-4.4.3-gitf80d6b9.tar.bz2
nacl-gcc.x86_64: W: no-manual-page-for-binary x86_64-nacl-gccbug
nacl-gcc.x86_64: W: no-manual-page-for-binary x86_64-nacl-gcc-4.4.3
nacl-gcc.x86_64: W: no-manual-page-for-binary x86_64-nacl-c++
nacl-gcc.x86_64: W: non-standard-dir-in-usr x86_64-nacl
All OK.

nacl-gcc.x86_64: E: script-without-shebang /usr/x86_64-nacl/lib64/libobjc.la
nacl-gcc.x86_64: E: script-without-shebang /usr/x86_64-nacl/lib64/libsupc++.la
nacl-gcc.x86_64: E: script-without-shebang /usr/x86_64-nacl/lib64/libstdc++.la
nacl-gcc.x86_64: E: script-without-shebang /usr/x86_64-nacl/lib32/libstdc++.la
nacl-gcc.x86_64: E: script-without-shebang /usr/x86_64-nacl/lib32/libobjc.la
nacl-gcc.x86_64: E: script-without-shebang /usr/x86_64-nacl/lib32/libsupc++.la
OK too. There's a rule against .la files, but a compiler is special.

3 packages and 0 specfiles checked; 7 errors, 509 warnings.

+ license is acceptable (GPLv3+ etc)
+ license file is present, %license is used
+ version is not the latest, but it should match chrome, so that's OK
+ builds and install OK
+ should not conflict normal gcc (custom prefix or suffix used everywhere)
+ no scriptlets present or necessary
+ provides/requires look sane
+ rpmlint has nothing interesting to say (above)

Package is approved.

Comment 5 Ralf Corsepius 2016-03-24 13:30:31 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #4)

> nacl-gcc.x86_64: E: script-without-shebang /usr/x86_64-nacl/lib64/libobjc.la
> nacl-gcc.x86_64: E: script-without-shebang
> /usr/x86_64-nacl/lib64/libsupc++.la
> nacl-gcc.x86_64: E: script-without-shebang
> /usr/x86_64-nacl/lib64/libstdc++.la
> nacl-gcc.x86_64: E: script-without-shebang
> /usr/x86_64-nacl/lib32/libstdc++.la
> nacl-gcc.x86_64: E: script-without-shebang /usr/x86_64-nacl/lib32/libobjc.la
> nacl-gcc.x86_64: E: script-without-shebang
> /usr/x86_64-nacl/lib32/libsupc++.la
> OK too. There's a rule against .la files, but a compiler is special.

Putting aside the fact that I do not see any sense in shipping these *.la's, 
the rpmlint warning typically means that these files carry bogus permissions (Likely these are marked "executable" while they should not be).

Comment 6 Zbigniew Jędrzejewski-Szmek 2016-03-24 14:04:59 UTC
True. Maybe it'd be better to remove them after all, and see if the compiler still works... If it does, that's probably the best solution.

Comment 7 Parag AN(पराग) 2016-04-05 04:17:59 UTC
Any update here, I see Zbigniew already approved this package but can't see its built in Fedora.

Comment 8 Gwyn Ciesla 2016-04-08 20:55:04 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/nacl-gcc

Comment 9 Fedora Update System 2016-04-11 21:22:43 UTC
nacl-newlib-2.1.0-9.20150528git8c4da47.fc24 nacl-gcc-4.4.3-18.20150504gitf80d6b9.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-3d8df355cf

Comment 10 Fedora Update System 2016-04-11 21:22:55 UTC
nacl-newlib-2.1.0-9.20150528git8c4da47.fc23 nacl-gcc-4.4.3-18.20150504gitf80d6b9.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-b28ac9a3e6

Comment 11 Fedora Update System 2016-04-11 21:23:04 UTC
nacl-newlib-2.1.0-9.20150528git8c4da47.fc22 nacl-gcc-4.4.3-18.20150504gitf80d6b9.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2016-26a2a4f89f

Comment 12 Fedora Update System 2016-04-12 10:52:24 UTC
nacl-gcc-4.4.3-18.20150504gitf80d6b9.fc24, nacl-newlib-2.1.0-9.20150528git8c4da47.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-3d8df355cf

Comment 13 Fedora Update System 2016-04-13 08:35:07 UTC
nacl-gcc-4.4.3-18.20150504gitf80d6b9.fc22, nacl-newlib-2.1.0-9.20150528git8c4da47.fc22 has been pushed to the Fedora 22 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-26a2a4f89f

Comment 14 Fedora Update System 2016-04-13 09:27:11 UTC
nacl-gcc-4.4.3-18.20150504gitf80d6b9.fc23, nacl-newlib-2.1.0-9.20150528git8c4da47.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-b28ac9a3e6

Comment 15 Fedora Update System 2016-04-15 21:33:11 UTC
nacl-gcc-4.4.3-18.20150504gitf80d6b9.fc24, nacl-newlib-2.1.0-9.20150528git8c4da47.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2016-04-26 20:54:14 UTC
nacl-gcc-4.4.3-18.20150504gitf80d6b9.fc23, nacl-newlib-2.1.0-9.20150528git8c4da47.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2016-04-26 21:21:24 UTC
nacl-gcc-4.4.3-18.20150504gitf80d6b9.fc22, nacl-newlib-2.1.0-9.20150528git8c4da47.fc22 has been pushed to the Fedora 22 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.