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

Split --disable-tests, fix cross builds #9611

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

Ericson2314
Copy link
Member

Motivation

It might seem obnoxious to have yet more configure flags, but I found controlling both the unit and functional tests with one flag was quite confusing because they are so different:

  • unit tests depending on building, functional tests don't (e.g. when we test already-built Nix)

  • unit tests can be installed, functional tests cannot

  • unit tests neeed extra libraries (GTest, RapidCheck), functional tests need extra executables (jq).

  • unit tests are run by make check, functional tests are run by make installcheck

Really on a technical level, they seem wholly independent. Only on a human level ("they are both are tests") do they have anything in common.

I had messed up the logic in cross builds because of this. Now I split the flag in two (and cleaned up a few other inconsistencies), and the logic fixed itself.

Context

I broke cross builds in #9535

Priorities

Add 👍 to pull requests you find important.

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Looks about right, and seems to simplify the docs build logic, which I of course approve.

Please someone else review it properly.

mk/disable/doc-gen.mk Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Dec 15, 2023
@Ericson2314
Copy link
Member Author

Discussed in Nix team meeting:

Solution to "disable makefile" controversy is just inlining them to where they are used, the top-level Makefile.

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.

Otherwise looks good.

configure.ac Outdated Show resolved Hide resolved
It might seem obnoxious to have yet more configure flags, but I found
controlling both the unit and functional tests with one flag was quite
confusing because they are so different:

- unit tests depending on building, functional tests don't (e.g. when
  we test already-built Nix)

- unit tests can be installed, functional tests cannot

- unit tests neeed extra libraries (GTest, RapidCheck), functional
  tests need extra executables (jq).

- unit tests are run by `make check`, functional tests are run by `make
  installcheck`

Really on a technical level, they seem wholly independent. Only on a
human level ("they are both are tests") do they have anything in common.

I had messed up the logic in cross builds because of this. Now I
split the flag in two (and cleaned up a few other inconsistencies), and
the logic fixed itself.

Co-Authored-By: Robert Hensing <roberth@users.noreply.github.com>
@roberth roberth merged commit 5d5b25f into NixOS:master Dec 18, 2023
8 checks passed
@thufschmitt
Copy link
Member

This fixed a CI failure (https://github.com/NixOS/nix/actions/runs/7204404968/job/19626137839, complaining about a failing jq in a cross job), but caused another one (https://github.com/NixOS/nix/actions/runs/7250906152/job/19753759380, complaining about make check ran while the tests were disabled).

@Ericson2314 can you take a look?

@Ericson2314 Ericson2314 deleted the fix-cross-configure branch December 20, 2023 07:36
@Ericson2314
Copy link
Member Author

#9646 Fixed here @thufschmitt .

@wegank wegank mentioned this pull request Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants