gcc-arm-embedded: 6-2017-q2-update -> 7-2018-q2-update #47542

prusnak · 2018-09-29T22:05:34Z

Motivation for this change

gcc-arm-embedded was not updated since mid-2017, this update brings the package to the latest possible version

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

prusnak · 2018-09-29T22:07:14Z

I would suggest the inclusion of this change into 18-09 as well.

tomberek · 2018-09-30T01:03:10Z

nox-review pr 47542 on NixOS

...
gdb_main.c: In function 'gdb_main_loop':
gdb_main.c:157:16: error: this statement may fall through [-Werror=implicit-fallthrough=]
    single_step = true;
                ^
gdb_main.c:159:3: note: here
   case 'c': /* 'c [addr]': Continue [at addr] */
   ^~~~
gdb_main.c:167:16: error: this statement may fall through [-Werror=implicit-fallthrough=]
    single_step = false;
                ^
gdb_main.c:169:3: note: here
   case '?': { /* '?': Request reason for target halt */
   ^~~~
%% bus_spi.c
cc1: all warnings being treated as errors
make: *** [Makefile:71: gdb_main.o] Error 1
%% piniobox.c
...
builder for '/nix/store/cc4pdza1zrk0izxbnqcgrjb19yl8zi0f-blackmagic-1.6.1.drv' failed with exit code 2             
error: build of '/nix/store/5pzgcm2wma2lviibja7km4w3szciw13v-inav-2.0.0-rc2.drv', '/nix/store/cc4pdza1zrk0izxbnqcgrjb19yl8zi0f-blackmagic-1.6.1.drv', '/nix/store/xmir01cxxamlpqslgp4p9cgfg643q9c7-betaflight-3.4.0-rc4.drv', '/nix/store/y4m4a0s68bqzwlwl5w55h13jhrqidkgf-axoloti-1.0.12-1.drv', '/nix/store/y6g3l860w20spam79a8hgnny1mxdpb1y-opentx-2.2.1.drv' failed

jb55 · 2018-09-30T02:05:00Z

pkgs/top-level/all-packages.nix

@@ -6744,7 +6744,8 @@ with pkgs;
    ncurses = pkgsi686Linux.ncurses5;
  };
  gcc-arm-embedded-6 = callPackage ../development/compilers/gcc-arm-embedded/6 {};
-  gcc-arm-embedded = gcc-arm-embedded-6;
+  gcc-arm-embedded-7 = callPackage ../development/compilers/gcc-arm-embedded/7 {};
+  gcc-arm-embedded = gcc-arm-embedded-7;


it looks like there are some breakages. perhaps we could keep this at 6 for now?

Updated in eaa8bb5be5e7b41ff1fe4aa6bc104c010ccfe1f8

prusnak · 2018-09-30T08:26:13Z

@tomberek addressed in eaa8bb5be5e7b41ff1fe4aa6bc104c010ccfe1f8 by keeping the default to gcc-arm-embedded-6. Cleaner option would be set 7 as default and fix the packages built using gcc-arm-embedded, there are not too many (gnuk, blackmagic, betaflight, inav, opentx, axoloti), but I don't want to touch other's people packages now as I am quite new to this. Third option is to set dependencies of mentioned packages to "gcc-arm-embedded-6" instead of "gcc-arm-embedded". What do you think?

jb55 · 2018-09-30T14:40:40Z

Third option is to set dependencies of mentioned packages to "gcc-arm-embedded-6" instead of "gcc-arm-embedded"

sounds good to me

tomberek · 2018-09-30T16:11:20Z

Provide feedback to the other packages so they are aware of the issue and can fix it. People seem to welcome this.

Mic92 · 2018-09-30T21:54:06Z

pkgs/development/compilers/gcc-arm-embedded/7/default.nix

+    find $out -type f | while read f; do
+      patchelf $f > /dev/null 2>&1 || continue
+      patchelf --set-interpreter $(cat ${stdenv.cc}/nix-support/dynamic-linker) "$f" || true
+      patchelf --set-rpath ${stdenv.lib.makeLibraryPath [ "$out" stdenv.cc.cc ncurses5 python27 ]} "$f" || true


Please consider using autoPatchelfHook. This will make updating a bit easier and is more robust then the approach chosen here.

Adding all libraries to buildInputs will enable the hook to add it to the RPATH.

From your comments, I figured out I should do the following, but should I also modify the find in preFixup or remove the preFixup completely (+ removing fixupPhase from phases)? Sorry, I'm quite new to this :-)

--- a/pkgs/development/compilers/gcc-arm-embedded/7/default.nix +++ b/pkgs/development/compilers/gcc-arm-embedded/7/default.nix @@ -1,4 +1,4 @@ -{ stdenv, fetchurl, ncurses5, python27 }: +{ stdenv, fetchurl, ncurses5, python27, autoPatchelfHook }: stdenv.mkDerivation rec { name = "gcc-arm-embedded-${version}"; @@ -24,6 +24,10 @@ stdenv.mkDerivation rec { cp -r * $out ''; + nativeBuildInputs = [ autoPatchelfHook ]; + + buildInputs = [ stdenv.cc.cc ncurses5 python27 ]; + dontPatchELF = true; dontStrip = true;

In the best case preFixup should become unnecessary. You can also remove the explicit phases assignment, it actually a bit of an anti-pattern to skip phases since users might expect them. You only ever need to specify an installPhase unless there is an make install. dontPatchELF and dontStrip can also go.

I am unable to get the build working using the changes you suggest, but I pushed them to a separate gcc-arm-embedded_autopatchelfhook branch, so I can return to that later. I don't want to dig into this any further, I just want to update the package and fix the blackmagic breakage for now.

fair enough.

cc @dezgeg it currently fails on

$ patchelf --remove-rpath /nix/store/0bvwvmaz3rd8iz3zc3vyclwid0sfbbhg-gcc-arm-embedded-7-2018-q2-update/arm-none-eabi/lib/redboot-crt0.o wrong ELF type

$ file /nix/store/0bvwvmaz3rd8iz3zc3vyclwid0sfbbhg-gcc-arm-embedded-7-2018-q2-update/arm-none-eabi/lib/redboot-crt0.o /nix/store/0bvwvmaz3rd8iz3zc3vyclwid0sfbbhg-gcc-arm-embedded-7-2018-q2-update/arm-none-eabi/lib/redboot-crt0.o: ELF 32-bit LSB relocatable, ARM, EABI5 version 1 (SYSV), not stripped

https://dl.thalheim.io/e2GdCQGJnobRgFI7F3B73w/auto-patchelf-hook-error.log

I thought I saw some architecture related code in the hook. But I suppose this is not handled yet right?

I suppose that's the reason why it is handled manually in the original script and failures are silently skipped. We don't want to patch stuff that is part of the package, such as lib/redboot-crt0.o which is ELF 32-bit LSB relocatable, ARM, EABI5 version 1 (SYSV), not stripped and thus should not be processed via patchElf.

prusnak · 2018-10-01T20:23:16Z

@tomberek I was able to investigate the gcc-arm-embedded-7 compatibility further and found out that the only package that failed with the new gcc-arm-embedded-7 is blackmagic. Luckily, this issue was fixed in their git, so I updated it to the latest revision. Are you OK with that @pjones? This is applied in 4b4e256ed29b472dc7d189dd38c115cf3ad0e6d8, which now passed the nox-review.

pjones · 2018-10-01T20:48:57Z

@prusnak Sound good to me. Thanks for updating blackmagic.

pkgs/development/compilers/gcc-arm-embedded/7/default.nix

Mic92 · 2018-10-02T11:05:33Z

pkgs/development/compilers/gcc-arm-embedded/7/default.nix

+
+  urlString = "https://developer.arm.com/-/media/Files/downloads/gnu-rm/${subdir}/gcc-arm-none-eabi-${version}-linux.tar.bz2";
+
+  src = fetchurl { url=urlString; sha256="0sgysp3hfpgrkcbfiwkp0a7ymqs02khfbrjabm52b5z61sgi05xv"; }


trailing ; is missing.

src = fetchurl { url = "https://developer.arm.com/-/media/Files/downloads/gnu-rm/${subdir}/gcc-arm-none-eabi-${version}-linux.tar.bz2"; sha256="0sgysp3hfpgrkcbfiwkp0a7ymqs02khfbrjabm52b5z61sgi05xv"; };

Fixed in b3531b9

+ update blackmagic to latest commit which includes gcc7 fixes

Mic92 · 2018-10-03T16:29:34Z

Thanks!

prusnak · 2018-10-03T16:45:39Z

@Mic92 Thanks for the merge. What is the correct process to get this into 18.09 release?

Mic92 · 2018-10-03T17:35:04Z

I don't think we should backport gcc-arm-embedded = gcc-arm-embedded-7; since it is already late.
It should be ok to backport it as a new package. In that case open a new pull request targeting release-18.09 branch.

Mic92 · 2018-10-03T17:35:35Z

Use git cherry-pick -x for backporting and then git commit --amend to remove the gcc-arm-embedded = gcc-arm-embedded-7;

jb55 approved these changes Sep 29, 2018

View reviewed changes

GrahamcOfBorg added 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Sep 29, 2018

jb55 reviewed Sep 30, 2018

View reviewed changes

prusnak force-pushed the gcc-arm-embedded_update branch from 1c32e83 to eaa8bb5 Compare September 30, 2018 08:22

Mic92 reviewed Sep 30, 2018

View reviewed changes

prusnak force-pushed the gcc-arm-embedded_update branch from eaa8bb5 to 4b4e256 Compare October 1, 2018 20:20

Mic92 reviewed Oct 2, 2018

View reviewed changes

pkgs/development/compilers/gcc-arm-embedded/7/default.nix Outdated Show resolved Hide resolved

prusnak force-pushed the gcc-arm-embedded_update branch from 4b4e256 to 04604e0 Compare October 2, 2018 10:55

Mic92 reviewed Oct 2, 2018

View reviewed changes

gcc-arm-embedded: 6-2017-q2-update -> 7-2018-q2-update

b3531b9

+ update blackmagic to latest commit which includes gcc7 fixes

prusnak force-pushed the gcc-arm-embedded_update branch from 04604e0 to b3531b9 Compare October 2, 2018 13:23

GrahamcOfBorg added the 8.has: clean-up label Oct 2, 2018

Mic92 merged commit 50a3061 into NixOS:master Oct 3, 2018

prusnak deleted the gcc-arm-embedded_update branch October 3, 2018 16:45

prusnak restored the gcc-arm-embedded_update branch October 3, 2018 16:45

prusnak deleted the gcc-arm-embedded_update branch October 3, 2018 17:44

prusnak mentioned this pull request Oct 3, 2018

gcc-arm-embedded: 6-2017-q2-update -> 7-2018-q2-update #47788

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcc-arm-embedded: 6-2017-q2-update -> 7-2018-q2-update #47542

gcc-arm-embedded: 6-2017-q2-update -> 7-2018-q2-update #47542

prusnak commented Sep 29, 2018 •

edited

prusnak commented Sep 29, 2018

tomberek commented Sep 30, 2018

jb55 Sep 30, 2018

prusnak Sep 30, 2018

prusnak commented Sep 30, 2018 •

edited

jb55 commented Sep 30, 2018

tomberek commented Sep 30, 2018

Mic92 Sep 30, 2018

Mic92 Sep 30, 2018

prusnak Sep 30, 2018 •

edited

Mic92 Oct 1, 2018 •

edited

prusnak Oct 2, 2018

Mic92 Oct 2, 2018

Mic92 Oct 2, 2018

prusnak Oct 2, 2018 •

edited

prusnak commented Oct 1, 2018 •

edited

pjones commented Oct 1, 2018

Mic92 Oct 2, 2018

prusnak Oct 2, 2018

Mic92 commented Oct 3, 2018

prusnak commented Oct 3, 2018

Mic92 commented Oct 3, 2018

Mic92 commented Oct 3, 2018


		urlString = "https://developer.arm.com/-/media/Files/downloads/gnu-rm/${subdir}/gcc-arm-none-eabi-${version}-linux.tar.bz2";

		src = fetchurl { url=urlString; sha256="0sgysp3hfpgrkcbfiwkp0a7ymqs02khfbrjabm52b5z61sgi05xv"; }

gcc-arm-embedded: 6-2017-q2-update -> 7-2018-q2-update #47542

gcc-arm-embedded: 6-2017-q2-update -> 7-2018-q2-update #47542

Conversation

prusnak commented Sep 29, 2018 • edited

Motivation for this change

Things done

prusnak commented Sep 29, 2018

tomberek commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusnak commented Sep 30, 2018 • edited

jb55 commented Sep 30, 2018

tomberek commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusnak Sep 30, 2018 • edited

Choose a reason for hiding this comment

Mic92 Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusnak Oct 2, 2018 • edited

Choose a reason for hiding this comment

prusnak commented Oct 1, 2018 • edited

pjones commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 commented Oct 3, 2018

prusnak commented Oct 3, 2018

Mic92 commented Oct 3, 2018

Mic92 commented Oct 3, 2018

prusnak commented Sep 29, 2018 •

edited

prusnak commented Sep 30, 2018 •

edited

prusnak Sep 30, 2018 •

edited

Mic92 Oct 1, 2018 •

edited

prusnak Oct 2, 2018 •

edited

prusnak commented Oct 1, 2018 •

edited