search

PetarKirov / dlang.nix

Nix expressions for building D compilers
MIT License
6 stars 2 forks source link

feat(pkgs/dmd): Version 2.092.1 supported #16

Closed dukc closed 6 months ago

dukc commented 10 months ago

Also fixed some build errors for 2.088.1, but it doesn't yet build with these since we're using a newer bootstrap compiler than OldDDerivations did.

Note: I'm targeting #9 as opposed to main, so that we don't have to rebase one branch on top of the other later.

dukc commented 7 months ago

Needs to be moved to supported-source-versions.json instead of using the .nix file. Converting to draft until done.

dukc commented 6 months ago

@PetarKirov updated!

PetarKirov commented 6 months ago

Also fixed some build errors for 2.088.1, but it doesn't yet build with these since we're using a newer bootstrap compiler than OldDDerivations did.

We can add a function which given a compiler version returns the bootstrap version that should be used to build it. I assume that we will need to do this at some point (even if the reason is not supporting old versions) as newer compiler versions may need to be bootstrapped with new versions themselves.

PetarKirov commented 6 months ago

@PetarKirov updated

Thanks @dukc, but can you rebase this PR and resolve the conflicts, since I merged the 2.096.1 one?

Additionally, can you see if you can use (and extend if necessary) the build-status infrastructure? See this commit for more details: https://github.com/PetarKirov/dlang.nix/commit/cf432d07e3b922639e9bcc54315afde5fd34c5e5

You can find another example of using in PR #40: https://github.com/PetarKirov/dlang.nix/pull/40/commits/9bb7e2a44e129556be98364aba35961f27140f5f

dukc commented 6 months ago

Thanks @dukc, but can you rebase this PR and resolve the conflicts, since I merged the 2.096.1 one?

@PetarKirov Yes, done and thanks.

Additionally, can you see if you can use (and extend if necessary) the build-status infrastructure? See this commit for more details:

I don't have a Darwin machine (perhaps I should...) so can't test what I add. I settled on copying 2.096.1 darwin config for 2.092.1.

You can find another example of using in PR

I checked and noticed we're using build-status to skip runnable_cxx/cppa.d for 2.103.1 and newer (for darwin), but the generic.nix file to skip the test, for all platforms, on earlier versions. If I understood it right, we would ideally define generic.nix so that we don't need to have anything in this file, meaning that file serves as a machine-checked bug tracker. Right?

dukc commented 6 months ago

so can't test what I add

Stupid me, I can check the Github CI results for that.

PetarKirov commented 6 months ago

I'm working on moving all version-conditional logical about skipping tests to a build-status.nix file, to help keep the derivations clean.

dukc commented 6 months ago

I'm working on moving all version-conditional logical about skipping tests to a build-status.nix file, to help keep the derivations clean.

I agree in half. IMO, the question is do we want to make the test in question to work in the future? If we do, build-status.nix is a fine place for it but if it's not worth to fix it's better to keep it in the derivation. In the latter case, removal of the test is in essence a compatibility patch.

dukc commented 6 months ago

Was changing the hash format myself, got merge conflicts and noticed you have already fixed it yourself, and on top of that done a boatload of other changes to build-status.nix and (i think) rebased this whole thing. In essence, you commandeered this whole PR -- maybe I should start reviewing this from now on instead :+1: .

PetarKirov commented 6 months ago

I have several goals (that are at odds with each other):

  • Support as many versions as possible
  • Minimize skipped tests

Given these two goals alone, I expect the list of comparability ~cruft~ patches to be ever expanding. For example some of the runnable_cxx tests work only with specific g++ compiler versions.

the question is do we want to make the test in question to work in the future?

It may be too ambitious, but yes. If at the time of a given pull request we can't make a given test work we will leave it disabled, but if possible, it would be nice if we could make it work at a later time. After all, all DMD releases have their test suites passing at 100%. Surely, with the power of Nix reproducibility we should be able to achieve the same. Well at least in theory. In practice, for the following examples I'd say:

PetarKirov commented 6 months ago

Btw, I'm not satisfied with the build-status.nix name, so suggestions are welcome. Basically the idea is two fold:

PetarKirov commented 6 months ago

Was changing the hash format myself, got merge conflicts and noticed you have already fixed it yourself, and on top of that done a boatload of other changes to build-status.nix and (i think) rebased this whole thing. In essence, you commandeered this whole PR -- maybe I should start reviewing this from now on instead 👍 .

Yes, sorry for not being clear about my intentions. I did the first batch of changes before going to bed, but I really should've said that I've already done the changes that I requested.

In my view, you did a good enough job so far with this PR, so I didn't want to ask for more with what I had planned about build-status.nix.

github-actions[bot] commented 6 months ago

Thanks for your Pull Request!

Below you will find a summary of the cachix status of each package, for each supported platform.

package x86_64-linux x86_64-darwin aarch64-darwin
dmd ✅ cached ✅ cached 🚫 not supported
dmd-2_092_1 ✅ cached ✅ cached 🚫 not supported
dmd-2_096_1 ✅ cached ✅ cached 🚫 not supported
dmd-2_098_1 ✅ cached ✅ cached 🚫 not supported
dmd-2_100_2 ✅ cached ✅ cached 🚫 not supported
dmd-2_102_2 ✅ cached ✅ cached 🚫 not supported
dmd-2_103_1 ✅ cached ✅ cached 🚫 not supported
dmd-2_104_2 ✅ cached ✅ cached 🚫 not supported
dmd-2_105_2 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_079_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_080_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_081_2 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_082_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_083_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_084_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_085_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_086_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_087_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_088_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_089_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_090_1 ✅ cached ✅ cached 🚫 not supported
dmd-binary-2_098_0 ✅ cached ✅ cached 🚫 not supported
dmd-bootstrap ✅ cached ✅ cached 🚫 not supported
dub ✅ cached ✅ cached ✅ cached
dub-1_30_0 ✅ cached ✅ cached ✅ cached
ldc ✅ cached 🚧 known to fail (disabled) 🚧 known to fail (disabled)
ldc-1_30_0 ✅ cached 🚧 known to fail (disabled) 🚧 known to fail (disabled)
ldc-binary ✅ cached ✅ cached ✅ cached
ldc-binary-1_19_0 ✅ cached ✅ cached 🚫 not supported
ldc-binary-1_25_0 ✅ cached ✅ cached ✅ cached
ldc-binary-1_28_0 ✅ cached ✅ cached ✅ cached
ldc-binary-1_32_1 ✅ cached ✅ cached ✅ cached
ldc-binary-1_34_0 ✅ cached ✅ cached ✅ cached
PetarKirov commented 6 months ago

@dukc The pipeline is now green after my build-status.nix refactoring (7c4e0169f9a7b596a91f1b7d931b8a9147408bcd). I had to add a patch to fix the compilation of dmd 2.102.2 - 2.104.0-beta.1 - see 9a750cdb105645cb14c60af13e919b5d5f13275c.

I'll now go ahead and merge this PR (as I have taken long enough to do it already!), but I'd appreciate if you could review these two commits, if you have the time.

dukc commented 6 months ago

t may be too ambitious, but yes. If at the time of a given pull request we can't make a given test work we will leave it disabled, but if possible, it would be nice if we could make it work at a later time. After all, all DMD releases have their test suites passing at 100%. Surely, with the power of Nix reproducibility we should be able to achieve the same. Well at least in theory. In practice, for the following examples I'd say:

I don't see any value in the ddocYear test for us - let's keep it indefinitely skipped GDB tests - while it seems that they cause only trouble for our Nix packaging efforts, I do think we should make them work, as we should have assurance that DMD built with Nix produces programs with working debug info.

The Ddoc year test was in fact precisely what I was thinking as an example of what we don't want to clutter the derivation for, more than it takes to disable it at or before the last time it was hardcoded. But for anything else, if in doubt let's put it to build-status.nix. The reasoning being, anything in build-status.nix is an "open bug issue", so any build failure should go there by default. Only if we explicitly decide we don't want to fix it the patch should move to the main derivation.

dukc commented 6 months ago

Btw, I'm not satisfied with the build-status.nix name, so suggestions are welcome. Basically the idea is two fold:

To tell if a given package version on a given platform is expected to:
   build successfully
   test successfully

To move all of the version conditional logic for removing tests out of the derivation

I think the name is fine. If you want an alternative, maybe build-expectations.nix?

And as I wrote above, I think most of the version conditions that disable tests (as opposed to setting compiler flags of patching the source) actually do belong there. For the remaining patches, splitting the patches to another file, say versioned-patches.nix is my suggestion, if having them in generic.nix starts hurting the eyes.

dukc commented 6 months ago

Reviewed.

All good. Not only that, they are in line of what I advocated for (test patches in build-status.nix, other patches still in generic.nix)! Only improvement idea I have is renaming version-utils.nix to utils.nix. We probably aren't going to have a lot of custom version functions so I'd rather bundle them with any other utils we might write.

dukc commented 6 months ago

Forgot to ping @PetarKirov .

PetarKirov commented 6 months ago

Reviewed.

Thanks!

I think most of the version conditions that disable tests [..] actually do belong there.

:+1:

anything in build-status.nix is an "open bug issue", so any build failure should go there by default.

Exactly!

Only if we explicitly decide we don't want to fix it the patch should move to the main derivation

To be honest, I'd prefer to keep all of the disabled tests out of the derivation. If you prefer, we can have something like skippedTests = alwaysDisabled ++ needInvestigation to better separate the open issues.

For the remaining patches, splitting the patches to another file, say versioned-patches.nix is my suggestion, if having them in generic.nix starts hurting the eyes.

Yeah, I was thinking the same as well.

I think the name is fine. If you want an alternative, maybe build-expectations.nix?

I don't have strong leanings in either direction, so let's leave it as it is. Of course, we can revisit this in the future.

PetarKirov commented 6 months ago

@dukc Btw, I finally managed to do the Nixpkgs upgrade (2023-11-04 -> 2024-02-16) in #49. The patch I added in this PR (9a750cdb105645cb14c60af13e919b5d5f13275c) certainly helped, but unfortunately, I had to disable additional C++ ABI tests in 75a2de549014bb1219957a10da778e7b2df967e8. The main change to darwin stdenv in Nixpkgs during this period was the clang 11 -> clang 16 bump (https://github.com/NixOS/nixpkgs/issues/234710). Unfortunately, the issues with clang 16 are not surprising, as upstream DMD tests only clang versions v8 - v13.