Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Feb 18, 2024
Merged

Conversation

dukc
Copy link
Collaborator

@dukc dukc commented Oct 14, 2023

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.

@PetarKirov PetarKirov force-pushed the refactor/gen-pkg-versions branch from 7733c3d to b90d46d Compare December 4, 2023 09:53
@PetarKirov PetarKirov force-pushed the refactor/gen-pkg-versions branch 12 times, most recently from d884879 to d5da1a5 Compare January 28, 2024 18:00
Base automatically changed from refactor/gen-pkg-versions to main January 28, 2024 19:46
@dukc
Copy link
Collaborator Author

dukc commented Jan 29, 2024

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

@dukc dukc marked this pull request as draft January 29, 2024 21:03
@dukc dukc force-pushed the olddderivations-port-4 branch from 19ac974 to 94561da Compare February 3, 2024 21:16
@dukc
Copy link
Collaborator Author

dukc commented Feb 3, 2024

@PetarKirov updated!

@dukc dukc marked this pull request as ready for review February 3, 2024 21:18
@PetarKirov
Copy link
Owner

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
Copy link
Owner

PetarKirov commented Feb 5, 2024

@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: cf432d0

You can find another example of using in PR #40: 9bb7e2a

@dukc dukc force-pushed the olddderivations-port-4 branch from 94561da to ea8a16d Compare February 8, 2024 16:09
@dukc
Copy link
Collaborator Author

dukc commented Feb 8, 2024

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
Copy link
Collaborator Author

dukc commented Feb 8, 2024

so can't test what I add

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

@PetarKirov PetarKirov force-pushed the olddderivations-port-4 branch from cc7aa39 to c01e713 Compare February 12, 2024 21:40
@PetarKirov
Copy link
Owner

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

@PetarKirov PetarKirov force-pushed the olddderivations-port-4 branch 2 times, most recently from c9fcb1f to d2d518c Compare February 13, 2024 11:00
@dukc
Copy link
Collaborator Author

dukc commented Feb 14, 2024

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
Copy link
Collaborator Author

dukc commented Feb 14, 2024

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 👍 .

@PetarKirov
Copy link
Owner

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

  • Support as many versions as possible
  • Support as many (optional) features as possible
    • switching bootstap compiler - LDC and GDC should be supported in addition to any recent DMD version)
    • LTO (if compiled with LDC or GDC)
    • Code coverage
    • assertions & debug info - helpful for troubleshooting compiler bugs
    • extra compiler flags (e.g. compiling druntime and phobos with -preview=in and -checkaction=context)
  • Minimize skipped tests
  • Have the derivation source code (pkgs/dmd/generic.nix) be as clean, understandable and maintainable as possible.

  • 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:

  • 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.

@PetarKirov
Copy link
Owner

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

@PetarKirov
Copy link
Owner

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.

@PetarKirov PetarKirov force-pushed the olddderivations-port-4 branch 4 times, most recently from 59a9320 to 644b8f1 Compare February 18, 2024 00:23
PetarKirov and others added 4 commits February 18, 2024 17:33
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.
And move all skipped tests from dmd's derivation to to
pkgs/dmd/build-status.nix.
@PetarKirov PetarKirov force-pushed the olddderivations-port-4 branch 2 times, most recently from d14624c to ab36881 Compare February 18, 2024 17:26
@PetarKirov PetarKirov force-pushed the olddderivations-port-4 branch from ab36881 to 50062d0 Compare February 18, 2024 17:32
Copy link

github-actions bot commented Feb 18, 2024

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
Copy link
Owner

PetarKirov commented Feb 18, 2024

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

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.

@PetarKirov PetarKirov merged commit 9a750cd into main Feb 18, 2024
2 checks passed
@PetarKirov PetarKirov deleted the olddderivations-port-4 branch February 18, 2024 17:33
@dukc
Copy link
Collaborator Author

dukc commented Feb 19, 2024

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
Copy link
Collaborator Author

dukc commented Feb 19, 2024

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
Copy link
Collaborator Author

dukc commented Feb 19, 2024

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
Copy link
Collaborator Author

dukc commented Feb 19, 2024

Forgot to ping @PetarKirov .

@PetarKirov
Copy link
Owner

Reviewed.

Thanks!

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

👍

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
Copy link
Owner

PetarKirov commented Feb 19, 2024

@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 (9a750cd) certainly helped, but unfortunately, I had to disable additional C++ ABI tests in 75a2de5. The main change to darwin stdenv in Nixpkgs during this period was the clang 11 -> clang 16 bump (NixOS/nixpkgs#234710). Unfortunately, the issues with clang 16 are not surprising, as upstream DMD tests only clang versions v8 - v13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants