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

Add buildDubPackage and dub-to-nix for building dub based packages #299618

Merged
merged 7 commits into from
Apr 4, 2024

Conversation

TomaSajt
Copy link
Contributor

@TomaSajt TomaSajt commented Mar 28, 2024

Description of changes

Closes #301302

This PR adds three main things:

  • the buildDubPackage helper function
  • the dub-to-nix helper script
  • the makeDubDep utility function to be used inside the files generated by dub-to-nix

The idea first came up in #288841 where I ended up implementing it. I though it would be a better idea to separate those changes into different PR, which is this one.

I also ended up migrating all 2 packages that use dub inside of nixpkgs.
Literate didn't even have anything to lock with dub-to-nix, so it has an empty dependency list.
The changes done to btdu is much more visible, though. The package declaration became much shorter, even when it only had 4 packages to lock.

I removed btdu's update script for now, maybe when I implement something like passthru.fetch-deps from buildDotnetModule for buildDubPackage I'll add a new one back.

I inited the serve-d package in this PR too to showcase that this works with many dependencies too.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a fine first iteration.

Could you somehow make clear that you (and perhaps @jtbx too) maintain the dlang support? A comment on top of the files or even setting yourself as the codeowner.

Some smaller things and questions:

pkgs/build-support/dlang/dub-to-nix/default.nix Outdated Show resolved Hide resolved

installPhase = "install -D bin/lit $out/bin/lit";
doCheck = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how the other derivation wrappers do this but if this is this generally desirable for dub packages, perhaps it could be enabled by default.

Comment on lines 29 to 33
checkPhase = ''
runHook preCheck
dub test
runHook postCheck
'';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably make sense to have in buildDubPackage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to come up with a sane default once I get back home.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out Literate didn't actually have any tests, but had a Makefile entry for testing. I'll try to build serve-d to test if the checking logic actually works.

Comment on lines +35 to +31
installPhase = ''
runHook preInstall
install -Dm755 bin/lit -t $out/bin
runHook preInstall
'';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no standardised install command for dub?

Copy link
Member

@jtbx jtbx Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but I can file an issue asking about it. Such a feature would be useful outside of packaging anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There exist an optional targetPath value inside dub.sdl or dub.json, AFAICT defaulting to ./.. It's where it puts the final binaries and the files inside copyPath.

Perhaps some jq based manipulation could be done to the file to add/update it and set it to a fixed directory, though I am not certain if this is robust enough.
(You can convert from .sdl to .json with a dub command, so jq could be used in either case)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively this could be an argument to buildDubPackage

@jtbx
Copy link
Member

jtbx commented Mar 28, 2024

Could you somehow make clear that you (and perhaps @jtbx too) maintain the dlang support? A comment on top of the files or even setting yourself as the codeowner.

I would be happy to :)

@TomaSajt
Copy link
Contributor Author

Added a CODEOWNERS entry for D

By the way, what should we call this language internally? I am using dlangfor build-support, because IMO d is way too short.

@Atemu
Copy link
Member

Atemu commented Mar 28, 2024

Ugh, I just remembered an annoying limitation of GH CODEOWNER: You two must have commit access.

I'd say add a comment in the files instead.

@TomaSajt
Copy link
Contributor Author

Ugh, I just remembered an annoying limitation of GH CODEOWNER: You two must have commit access.

I'd say add a comment in the files instead.

That's a bit annoying, yes. I'll add some comments, then.

@TomaSajt
Copy link
Contributor Author

I added some extra builder options for makeDubPackage. Previously it was only dubFlags, but now we have dubFlags dubBuildFlags and dubTestFlags.
I also added combined*Deps to factor out most of the string interpolation part from the script definition.

I created pkgs/build-support/dlang/README.md with a list of maintainers.

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Mar 28, 2024

Okay, so I set doCheck to false by default instead of true, because most applications don't have proper testing, and the fallback action when running dub test without proper unittests being set up is not very desirable. People will have to opt-in to testing by setting doCheck = true;.

@imrying imrying mentioned this pull request Mar 29, 2024
13 tasks
@TomaSajt
Copy link
Contributor Author

TomaSajt commented Mar 30, 2024

is there a way to tell dub to respect $NIX_BUILD_CORES ?

there is --build-mode=singleFile --parallel which will separate the workload onto multiple cores, however we can't specify the exact number of cores.

Edit: after testing, it doesn't really make a difference, or just makes it worse (at least for me). If people really want to use this, they can just use dubFlags

@TomaSajt
Copy link
Contributor Author

Okay, I updated #288841 to the new lockfile format, here's how you could do extra dub dependencies:
https://github.com/NixOS/nixpkgs/blob/dae2ee5c84d647add18220d5dbf14a4d8784cfd3/pkgs/applications/misc/inochi2d/generic.nix#L62-L73

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Mar 30, 2024

Okay, I added both release notes and docs for the things added by this PR (mostly based off of the nim section).

Run nix-build nixos/release.nix -A manual.x86_64-linux for the release notes.
Run nix-build doc for the docs.

Copy link
Member

@symphorien symphorien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the documentation! I left a few comments

doc/languages-frameworks/dlang.section.md Show resolved Hide resolved
doc/languages-frameworks/dlang.section.md Outdated Show resolved Hide resolved
* `dubTestFlags ? []`: The flags to pass to `dub test`.
* `compiler ? ldc`: The D compiler to be used by `dub`.

## Lockfiles {#dub-lockfiles}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this section should go before the example. There are two steps: 1/generate the lock file, 2/ write nix code like in this example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just swapped the lockfile and the parameters part around.
I think new the comment above dubLock in the example is enough indication of what to read next.

doc/languages-frameworks/dlang.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/dlang.section.md Outdated Show resolved Hide resolved
@TomaSajt TomaSajt force-pushed the dub-support branch 5 times, most recently from 7546833 to e521bec Compare March 30, 2024 20:52
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of nixpkgs-review pr 299618 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
6 packages built:
  • Literate
  • btdu
  • dub-to-nix
  • serve-d
  • tests.haskell.cabalSdist.assumptionLocalHasDirectReference
  • tests.haskell.cabalSdist.localHasNoDirectReference

Successfully ran btdu.

Great work! I especially like the docs :)

Comment on lines +1 to +3
{
"dependencies": {}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be inlined or even just be the default of buildDubPackage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be inlined, but I don't think many packages have 0 dependencies. If we have a default, it won't show a missing attribute error, which is worse IMO.

@Atemu Atemu merged commit b136700 into NixOS:master Apr 4, 2024
31 checks passed
@TomaSajt
Copy link
Contributor Author

TomaSajt commented Apr 4, 2024

Thanks for all the help and reviews, everyone!
This has probably been my most complex contribution to date, glad to see it get merged.

I'll get back cleaning up my other PR now.

@TomaSajt TomaSajt mentioned this pull request Apr 4, 2024
13 tasks
@jtbx jtbx mentioned this pull request Apr 5, 2024
13 tasks
@TomaSajt TomaSajt deleted the dub-support branch April 21, 2024 20:10
@Atemu Atemu mentioned this pull request May 6, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: serve-d
6 participants