Updating, fixing and improving dmd #27743

ThomasMader · 2017-07-29T15:59:58Z

This is the continuation of #11327.

It contains the following changes:

Updated to newest version of dmd (2.075.1).
Fetch source archives from GitHub because the archives on the dlang.org homepage have missing files. (see https://issues.dlang.org/show_bug.cgi?id=17517)
Added a checkPhase to test the runtime and standard libraries.
Added curl dependency because the Standard Library Phobos needs it.
Added tzdata dependency because the datetime/timezone implementation in Phobos needs it.
Compile with -fPIC to fix problems with binutils >= 2.28.
Added shared library to output for Linux.
Added manpages to output.
Updating meta information.
Unified the current version and bootstrap version derivation code.
Built on platform(s)
- NixOS
- macOS

mention-bot · 2017-07-29T16:00:00Z

@ThomasMader, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vlstill, @johbo and @BlackEdder to be potential reviewers.

joachifm · 2017-07-31T15:28:46Z

Any idea what's up with the macos build error reported by travis?

ThomasMader · 2017-07-31T15:32:59Z

No wondering about it myself but Commit 740d763 seems strange to me.
the referenced commit in the commit message "ba68231273bea4cba01413fd2a0e56d68db9234c" looks different and it's a really big change with 132 files.

ThomasMader · 2017-07-31T15:38:44Z

@joachifm For me it looks like @FRidh wanted to cherry pick one commit from staging into master but instead merged an entire branch or something.

joachifm · 2017-07-31T15:40:53Z

Could be intended as a partial merge to reduce parallel rebuilds.

FRidh · 2017-07-31T15:43:14Z

Could be intended as a partial merge to reduce parallel rebuilds.

Exactly that.

ThomasMader · 2017-07-31T15:55:23Z

Nevertheless, my guess is that one of the changes on the wrapper in Commit 740d763 is the problem.

ThomasMader · 2017-08-01T15:49:33Z

I was able to verify that 740d763 contains the problem commit. Don't know the exact commit which broke the build of gcc-5.4.0.

@edolstra @Ericson2314 Any ideas? My guess is it's a change in the CC-wrapper stuff but don't know much about the nixpkgs structure.

I am currently building with reverted 47821f1 on my Mac Mini but building takes ages. :-(

Ericson2314 · 2017-08-01T16:12:07Z

Huh, this is odd. The errors look like you are missing libc. Can you go in a nix-shell and do something like echo '#include <stdlib.h>' | cc -E -v and checkout out the expansion / include paths?

ThomasMader · 2017-08-01T17:31:47Z

nix-shell . -A gcc5 fails with the same error as on the CI server.

nix-shell . -A clang_4
[nix-shell:~/devel/nixpkgs2]$ echo '#include <stdlib.h>' | cc -E -v
clang version 4.0.1 (tags/RELEASE_401/final)
Target: x86_64-apple-darwin15.6.0
Thread model: posix
InstalledDir: /nix/store/bcdqax78hm8xpcbh8rilx130qrk0x91a-clang-4.0.1/bin
clang-4.0: warning: argument unused during compilation: '-idirafter /nix/store/mr8bhbnyjph6lrmak0i1bq56pyv5vg7w-Libsystem-osx-10.11.6/include' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-idirafter /nix/store/bcdqax78hm8xpcbh8rilx130qrk0x91a-clang-4.0.1/lib/gcc///include-fixed' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-F/nix/store/szvfr1ygyj7vdm9zcxizki4g3qq28xzw-CF-osx-10.10.5/Library/Frameworks' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-isystem /nix/store/basva09sb917d741gdn89sxljyvs0h8n-libc++-4.0.1/include' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-O2' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-D _FORTIFY_SOURCE=2' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-fstack-protector-strong' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '--param ssp-buffer-size=4' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-fPIC' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-fno-strict-overflow' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-Wformat' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-Wformat-security' [-Wunused-command-line-argument]
clang-4.0: warning: argument unused during compilation: '-Werror=format-security' [-Wunused-command-line-argument]

ThomasMader · 2017-08-01T17:34:28Z

Here is the error message for nix-shell with gcc5:

../../gcc-5.4.0/libiberty/fibheap.c:220:30: error: use of undeclared identifier 'LONG_MIN'
if (okey == key && okey != FIBHEAPKEY_MIN)
^
../../gcc-5.4.0/libiberty/fibheap.c:38:24: note: expanded from macro 'FIBHEAPKEY_MIN'
#define FIBHEAPKEY_MIN LONG_MIN
^
../../gcc-5.4.0/libiberty/fibheap.c:261:36: error: use of undeclared identifier 'LONG_MIN'
fibheap_replace_key (heap, node, FIBHEAPKEY_MIN);
^
../../gcc-5.4.0/libiberty/fibheap.c:38:24: note: expanded from macro 'FIBHEAPKEY_MIN'
#define FIBHEAPKEY_MIN LONG_MIN
^
../../gcc-5.4.0/libiberty/fibheap.c:265:7: warning: implicitly declaring library function 'abort' with type 'void (void) attribute((noreturn))' [-Wimplicit-function-declaration]
abort ();
^
../../gcc-5.4.0/libiberty/fibheap.c:265:7: note: include the header <stdlib.h> or explicitly provide a declaration for 'abort'
../../gcc-5.4.0/libiberty/fibheap.c:368:3: warning: implicitly declaring library function 'memset' with type 'void *(void *, int, unsigned long)' [-Wimplicit-function-declaration]
memset (a, 0, sizeof (fibnode_t) * D);
^
../../gcc-5.4.0/libiberty/fibheap.c:368:3: note: include the header <string.h> or explicitly provide a declaration for 'memset'
4 warnings and 2 errors generated.
make[3]: *** [Makefile:752: fibheap.o] Error 1
make[3]: Leaving directory '/private/tmp/nix-build-gcc-5.4.0.drv-0/build/libiberty'
make[2]: *** [Makefile:9595: all-stage1-libiberty] Error 2
make[2]: Leaving directory '/private/tmp/nix-build-gcc-5.4.0.drv-0/build'
make[1]: *** [Makefile:19039: stage1-bubble] Error 2
make[1]: Leaving directory '/private/tmp/nix-build-gcc-5.4.0.drv-0/build'
make: *** [Makefile:19344: bootstrap] Error 2
builder for ‘/nix/store/67k9dxxarnjmn80x02ix4lcwha73mbq5-gcc-5.4.0.drv’ failed with exit code 2
error: build of ‘/nix/store/67k9dxxarnjmn80x02ix4lcwha73mbq5-gcc-5.4.0.drv’ failed
/nix/var/nix/profiles/default/bin/nix-shell: failed to build all dependencies

LnL7 · 2017-08-03T21:02:39Z

@ThomasMader FYI #27889

Ericson2314 · 2017-08-08T14:22:24Z

pkgs/development/compilers/dmd/default.nix

+    + stdenv.lib.optionalString stdenv.isDarwin ''
+        # Allow to use "clang++", commented in Makefile
+        substituteInPlace dmd/posix.mak \
+            --replace g++ clang++ \


You can replace with $(CXX) everywhere.

Done, thanks.

ThomasMader · 2017-08-10T06:12:18Z

I consider this done and ready to be merged.
The CI build is failing on Darwin for the dub derivation which depends on dmd and fails because of #27889.
I tested on Darwin without that problem and everything is fine.

Ericson2314 · 2017-08-11T13:26:52Z

pkgs/development/compilers/dmd/2.067.1.nix

      make -f posix.mak INSTALL_DIR=$out
-      export DMD=$PWD/dmd
+      ${
+          let bits = if stdenv.is64bit then "64" else "32";


You can get that more directly from stdenv.hostPlatform.parsed.

This is used nowhere and it isn't working for me.
So what do you mean exactly?
Using

let bits = stdenv.hostPlatform.parsed;

gives me

error: cannot coerce a set to a string

on ${bits}.
I found the bits defined in lib/systems/parse but don't know how to get them out of it with your call.

There's a bunch of things in that attrset. Try stdenv.hostPlatform.parsed.cpu.bits maybe?

Oh, and in general use nix-repl to explore hostPlatform and similar values.

Done, thanks.

Ericson2314 · 2017-08-11T13:28:55Z

pkgs/development/compilers/dmd/2.067.1.nix

-      export DMD=$PWD/dmd
+      ${
+          let bits = if stdenv.is64bit then "64" else "32";
+          osname = if stdenv.isDarwin then "osx" else "linux"; in


These seem unused / should be hoisted out due to repetition?

Done, thanks.

Ericson2314 · 2017-08-17T15:26:13Z

pkgs/development/compilers/dmd/default.nix

-      "cp generated/${osname}/release/${bits}/libphobos2.a $out/lib"
+      ${
+          let bits = builtins.toString stdenv.hostPlatform.parsed.cpu.bits;
+          osname = if stdenv.isDarwin then "osx" else "linux";


Thanks for making the other two fixes! Last thing would be using hostPlatform and not assuming Linux:

let bits = builtins.toString stdenv.hostPlatform.parsed.cpu.bits; osname = if stdenv.hostPlatform.isDarwin then "osx" else stdenv.hostPlatform.parsed.kernel.name; extension = if stdenv.hostPlatform.isDarwin then "a" else "{a,so}"; in ...

Now also fixed, thanks again.

One question. While working on those two derivations I sometimes made changes to them but nix-build -K -A dmd did not trigger a rebuild.
I was under the impression that changing the derivation file in any way should always lead to a new hash and therefore need to trigger a rebuild.
I then thought that maybe nix is expanding the script in any form and can therefore cancel out some changes without a rebuild.
Any way, this happend again with this last commit and I simply added two empty new lines somewhere to trigger a rebuild.

Your second there is correct---if the expression evaluates to the same value, there is no rebuild. stdenv.hostPlatform.parsed.kernel.name evaluates to "linux" on Linux, so there is no change!

Thanks so much for these fixes, but don't forgot the stdenv.hostPlatform.is* change. stdenv.is* will be deprecated soon as it is ambiguous.

Sorry, didn't saw your change there.
Fixed.

FRidh · 2017-08-17T17:45:06Z

Please squash commits

ThomasMader · 2017-08-18T06:43:53Z

@FRidh Squashed the small fixup commits into one.

Additionally updated to newest version 2.075.1.

FRidh · 2017-08-18T07:42:54Z

I still see 17 dmd commits.

ThomasMader · 2017-08-18T09:31:23Z

So you want me to squash all 17 commits into one?
For what reason?
Big commits are considered bad.

ThomasMader · 2017-08-18T10:00:09Z

@FRidh Looking through my commits, the only 2 commits which should be squashed are e35b67398308092d9c63bec40128ec5087187ed7 and e1a1ed3e1be9622f8fd8cc54c7703ba2002c2c98 .
But the others are not work-in-progress commits as mentioned in https://github.com/blog/2141-squash-your-commits .
And shouldn't you be able to squash and merge with the option explained in the blog article if you really wanted too?
But again, I really don't consider squashing all these commits into one as good pratice.

FRidh · 2017-08-18T10:38:11Z

So you want me to squash all 17 commits into one?

Yes

For what reason?
Big commits are considered bad.

Nixpkgs is a big repository with expressions for tens of thousands of packages, not a regular repository describing a single piece of software. Navigating the history is already though and having commits for details on how an expression changes is just noise. Its important that changes are bundled, e.g. when needing to revert them.

ThomasMader · 2017-08-18T11:52:20Z

@FRidh Squashed everything into one commit.

Checked the build on my Linux machine again because the CI isn't working at the moment.

ThomasMader · 2017-08-18T12:00:30Z

Nixpkgs is a big repository with expressions for tens of thousands of packages, not a regular repository describing a single piece of software. Navigating the history is already though and having commits for details on how an expression changes is just noise. Its important that changes are bundled, e.g. when needing to revert them.

@FRidh Wouldn't it make sense to enable the squashing option from GitHub mentioned in the blog for this repository in this case? (https://github.com/blog/2141-squash-your-commits)
This way the Merger could just decide if squashing all commits of a PR should be done or even be very strict about it and force this paradigm for all PRs independent of what the contributor has done.

FRidh · 2017-08-18T13:43:15Z

Yes, I can merge it here, but I cannot make sure the commit name is correct without cloning the repo.
In the end, its the responsibility of the contributor to make sure the contributions are according to the guidelines.

Ericson2314 · 2017-08-18T15:07:49Z

@FRidh is that an official policy? I see no problem with multiple commits with a merge commit---the merge commit alone can be reverted, undoing the entire PR.

FRidh · 2017-08-18T15:19:48Z

@Ericson2314 we have the policy to have a commit per logical change, which typically means a commit per package or a commit per major change, not 20 commits as it was.

Ericson2314 · 2017-08-18T17:12:30Z

@FRidh Right, that makes sense; 20 was certainly too many. But I think separating refactors from version bumps---which IIRC was some of the original motivation for multiple commits before more commits in response to reviews---is acceptable and a good idea too.

FRidh · 2017-08-18T17:42:37Z

If they work regardless of the version bump then absolutely.

ThomasMader · 2017-08-20T16:52:03Z

#28396 depends on the fixes in here.

@FRidh Is there still something missing or can it be merged?
I have created an issue for the failing CI. (see #28371)

AlexeyRaga · 2017-08-22T05:51:13Z

Is it fixed? Do I need to do anything special to have a fix? I am still having this error:

../../gcc-5.4.0/libiberty/fibheap.c:220:30: error: use of undeclared identifier 'LONG_MIN'
  if (okey == key && okey != FIBHEAPKEY_MIN)
                             ^
../../gcc-5.4.0/libiberty/fibheap.c:38:24: note: expanded from macro 'FIBHEAPKEY_MIN'
#define FIBHEAPKEY_MIN  LONG_MIN
                        ^
../../gcc-5.4.0/libiberty/fibheap.c:261:36: error: use of undeclared identifier 'LONG_MIN'
  fibheap_replace_key (heap, node, FIBHEAPKEY_MIN);
                                   ^
../../gcc-5.4.0/libiberty/fibheap.c:38:24: note: expanded from macro 'FIBHEAPKEY_MIN'
#define FIBHEAPKEY_MIN  LONG_MIN
                        ^

ThomasMader · 2017-08-22T19:43:36Z

@AlexeyRaga This was never a problem of this PR, see #27889 for details.

ThomasMader changed the title ~~Update dmd 2.070.2 -> 2.075.0~~ Updating and improving dmd Jul 30, 2017

Ericson2314 reviewed Aug 8, 2017

View reviewed changes

Ericson2314 reviewed Aug 11, 2017

View reviewed changes

Ericson2314 reviewed Aug 17, 2017

View reviewed changes

ThomasMader force-pushed the master branch from afd47d0 to a897d8a Compare August 18, 2017 06:40

dmd: Updating (2.070.2 -> 2.075.1), fixing and improving. PR NixOS#27743

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: 91BE884FBA4B591A

Learn about vigilant mode

0842a45

ThomasMader force-pushed the master branch from a897d8a to 0842a45 Compare August 18, 2017 11:37

ThomasMader changed the title ~~Updating and improving dmd~~ Updating, fixing and improving dmd Aug 18, 2017

ThomasMader mentioned this pull request Aug 20, 2017

dub: Updating (1.1.0 -> 1.5.0) and improving #28396

Merged

8 tasks

FRidh merged commit 1eb48d3 into NixOS:master Aug 20, 2017

Updating, fixing and improving dmd #27743

Updating, fixing and improving dmd #27743

Conversation

ThomasMader commented Jul 29, 2017 • edited Loading

mention-bot commented Jul 29, 2017

joachifm commented Jul 31, 2017

ThomasMader commented Jul 31, 2017

ThomasMader commented Jul 31, 2017

joachifm commented Jul 31, 2017

FRidh commented Jul 31, 2017

ThomasMader commented Jul 31, 2017

ThomasMader commented Aug 1, 2017

Ericson2314 commented Aug 1, 2017

ThomasMader commented Aug 1, 2017

ThomasMader commented Aug 1, 2017

LnL7 commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasMader commented Aug 10, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh commented Aug 17, 2017

ThomasMader commented Aug 18, 2017

FRidh commented Aug 18, 2017

ThomasMader commented Aug 18, 2017

ThomasMader commented Aug 18, 2017

FRidh commented Aug 18, 2017

ThomasMader commented Aug 18, 2017

ThomasMader commented Aug 18, 2017

FRidh commented Aug 18, 2017

Ericson2314 commented Aug 18, 2017

FRidh commented Aug 18, 2017

Ericson2314 commented Aug 18, 2017

FRidh commented Aug 18, 2017

ThomasMader commented Aug 20, 2017

AlexeyRaga commented Aug 22, 2017

ThomasMader commented Aug 22, 2017

ThomasMader commented Jul 29, 2017 •

edited

Loading