Skip to content

toml11: only build tests when they're enabled & use system doctest/nlohmann_json#450200

Merged
roberth merged 2 commits intoNixOS:staging-nixosfrom
getchoo-contrib:pkgs/toml11/no-submodules
Oct 19, 2025
Merged

toml11: only build tests when they're enabled & use system doctest/nlohmann_json#450200
roberth merged 2 commits intoNixOS:staging-nixosfrom
getchoo-contrib:pkgs/toml11/no-submodules

Conversation

@getchoo
Copy link
Member

@getchoo getchoo commented Oct 9, 2025

Note

This (indirectly) fixes the main NixOS/nix flake when building against current NixOS unstable - as they have been overriding the source, but don't include submodules. This leads to doctest/nlohmann_json not being found when building tests...which is exactly how I found this :(

Previously, upstream's submodules were used when building tests instead of our own versions of the libraries. nlohmann_json works OOTB, but doctest needed a small patch to the test files to be picked up

This also ensures that the aforementioned tests are only built when checks are actually enabled

Built against nixos-unstable c9b6fb7

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-nixos-tests This PR causes rebuilds for all NixOS tests and should normally target the staging branches. labels Oct 9, 2025
@nix-owners nix-owners bot requested a review from Chaostheorie October 9, 2025 07:09
@getchoo getchoo mentioned this pull request Oct 9, 2025
Merged
@Chaostheorie
Copy link
Contributor

Chaostheorie commented Oct 9, 2025

The changes itself lgtm, I will try to build nix, lix and toml11 to verify the changes and approve afterward. This will likely take up to tomorrow.

Scrumplex
Scrumplex requested changes Oct 9, 2025
View reviewed changes
@getchoo getchoo force-pushed the pkgs/toml11/no-submodules branch from 50cdb43 to 632f34f Compare October 11, 2025 06:16
@getchoo getchoo requested a review from Scrumplex October 11, 2025 06:17
@getchoo
Copy link
Member Author

getchoo commented Oct 12, 2025

Lix failed to build due to some of its Python dependencies, but I don't think the changes here would realistically impact its functionality (as this should functionally be a no-op on the final output)

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 450200 --package lix --package toml11 --package nix
Commit: 632f34f0c0ecd14d2d0ef76f55407d9647b6ada2

aarch64-linux

❌ 6 packages failed to build:
  • lix
  • lix.debug (lix.debug.debug, lix.debug.dev, lix.debug.devdoc, lix.debug.doc, lix.debug.man)
  • lix.dev (lix.dev.debug, lix.dev.dev, lix.dev.devdoc, lix.dev.doc, lix.dev.man)
  • lix.devdoc (lix.devdoc.debug, lix.devdoc.dev, lix.devdoc.devdoc, lix.devdoc.doc, lix.devdoc.man)
  • lix.doc (lix.doc.debug, lix.doc.dev, lix.doc.devdoc, lix.doc.doc, lix.doc.man)
  • lix.man (lix.man.debug, lix.man.dev, lix.man.devdoc, lix.man.doc, lix.man.man)
✅ 5 packages built:
  • nix
  • nix.dev (nix.dev.dev, nix.dev.doc, nix.dev.man)
  • nix.doc (nix.doc.dev, nix.doc.doc, nix.doc.man)
  • nix.man (nix.man.dev, nix.man.doc, nix.man.man)
  • toml11

@Chaostheorie
Copy link
Contributor

Chaostheorie commented Oct 12, 2025 •
edited
Loading

My builds failed to something similar and I haven't been able to chase it down further, at least the library itself remains function.

Marking as approved to continue staging cycle. I will try to look into lix when time allows.

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Oct 12, 2025
Scrumplex
Scrumplex approved these changes Oct 18, 2025
View reviewed changes
Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

I think we can afford to merge this into master. Rebuilds are <200 and only nixosTest.simple is touched

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Oct 18, 2025
@xokdvium
Copy link
Contributor

xokdvium commented Oct 18, 2025

only nixosTest.simple is touched

Isn't nix still a dependency of all nixos tests or did this change recently? Wouldn't this need to go through the nixos-staging branch?

@Chaostheorie
Copy link
Contributor

Chaostheorie commented Oct 18, 2025 •
edited
Loading

The last update was also against staging (#442682) though it likely caused more builds compared to this PR (if presumably @Scrumplex estimate about rebuilt packages is correct).

We can also target nixos-staging but any noticable toml11 changes will cause a lot of rebuilds either way.

@getchoo getchoo changed the base branch from staging to staging-nixos October 19, 2025 04:08
@nixpkgs-ci nixpkgs-ci bot closed this Oct 19, 2025
@nixpkgs-ci nixpkgs-ci bot reopened this Oct 19, 2025
github-actions[bot]

This comment was marked as resolved.

Sign in to view

@getchoo getchoo force-pushed the pkgs/toml11/no-submodules branch from 632f34f to e463f58 Compare October 19, 2025 04:12
@github-actions github-actions bot dismissed their stale review October 19, 2025 04:13

All good now, thank you!

@nixpkgs-ci nixpkgs-ci bot added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 19, 2025
@nix-owners nix-owners bot requested a review from ShamrockLee October 19, 2025 04:20
@nix-owners nix-owners bot requested review from infinisil and roberth October 19, 2025 04:20
@Scrumplex
Copy link
Member

Scrumplex commented Oct 19, 2025

I was going off the eval summary which only lists 189 packages of which nixosTssts.simple is the only NixOS test

@getchoo
Copy link
Member Author

getchoo commented Oct 19, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 450200 --package lix --package nix
Commit: e463f5829797978e60e6ae95dfa832a319adad52

aarch64-linux

✅ 10 packages built:
  • lix
  • lix.debug (lix.debug.debug, lix.debug.dev, lix.debug.devdoc, lix.debug.doc, lix.debug.man)
  • lix.dev (lix.dev.debug, lix.dev.dev, lix.dev.devdoc, lix.dev.doc, lix.dev.man)
  • lix.devdoc (lix.devdoc.debug, lix.devdoc.dev, lix.devdoc.devdoc, lix.devdoc.doc, lix.devdoc.man)
  • lix.doc (lix.doc.debug, lix.doc.dev, lix.doc.devdoc, lix.doc.doc, lix.doc.man)
  • lix.man (lix.man.debug, lix.man.dev, lix.man.devdoc, lix.man.doc, lix.man.man)
  • nix
  • nix.dev (nix.dev.dev, nix.dev.doc, nix.dev.man)
  • nix.doc (nix.doc.dev, nix.doc.doc, nix.doc.man)
  • nix.man (nix.man.dev, nix.man.doc, nix.man.man)

roberth
roberth approved these changes Oct 19, 2025
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Checked

  • doCheck = false; works
  • cross compilation
  • tests still run
  • compared the output: identical modulo store path (this would cause no rebuilds if ca-derivations were used)

@roberth roberth merged commit 028e1a5 into NixOS:staging-nixos Oct 19, 2025
27 of 31 checks passed
@getchoo getchoo deleted the pkgs/toml11/no-submodules branch October 20, 2025 06:24
@trofi
Copy link
Contributor

trofi commented Nov 5, 2025

I noticed that e463f58 exposes build failure of pkgsStatic.doctest package (a nixStatic dependency). Proposed a possible trivial fix:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roberth roberth roberth approved these changes

@Scrumplex Scrumplex Scrumplex approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

@ShamrockLee ShamrockLee Awaiting requested review from ShamrockLee

@infinisil infinisil Awaiting requested review from infinisil

+1 more reviewer

@Chaostheorie Chaostheorie Chaostheorie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. 10.rebuild-nixos-tests This PR causes rebuilds for all NixOS tests and should normally target the staging branches. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@getchoo @Chaostheorie @xokdvium @Scrumplex @trofi @roberth