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

No longer copy functional tests to the build dir #11792

Merged
merged 1 commit into from
Nov 3, 2024

Conversation

Ericson2314
Copy link
Member

Motivation

This should make _NIX_TEST_ACCEPT=1 work again, fixing #11369.

Context

Progress on #2503

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@Ericson2314 Ericson2314 requested a review from edolstra as a code owner November 3, 2024 03:41
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Nov 3, 2024
@Ericson2314
Copy link
Member Author

OK just need to fix the VM test, good!

@Ericson2314 Ericson2314 force-pushed the no-copy-functional-tests branch 2 times, most recently from 20fa5a6 to b3e845b Compare November 3, 2024 14:51
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.

❤️

Some of the refactor suggestions can be follow-ups; would be great to get this in asap.

mkdir -p "$start"
cp -r common common.sh ${config_nix} ./nested-sandboxing "$start"
cp "${_NIX_TEST_BUILD_DIR}/common/subst-vars.sh" "$start/common"
# N.B. redefine
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh just that those variables are already defined, and so I am redefining them

(mutating doesn't seem right since the scope is difference)

Comment on lines +16 to +17
_NIX_TEST_SOURCE_DIR="$start"
_NIX_TEST_BUILD_DIR="$start"
Copy link
Member

Choose a reason for hiding this comment

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

Do we want these to be the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -55,6 +55,10 @@ in
-e 's!nix_tests += test-libstoreconsumer\.sh!!' \
;

_NIX_TEST_SOURCE_DIR="$(realpath tests/functional)"
export _NIX_TEST_SOURCE_DIR
export _NIX_TEST_BUILD_DIR="''${_NIX_TEST_SOURCE_DIR}"
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct?

Copy link
Member Author

@Ericson2314 Ericson2314 Nov 3, 2024

Choose a reason for hiding this comment

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

Yeah, the old configured source has those coincide. I'll switch it over in the next PR.

Comment on lines +22 to +23
export _NIX_TEST_SOURCE_DIR=$PWD
export _NIX_TEST_BUILD_DIR=$PWD
Copy link
Member

Choose a reason for hiding this comment

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

Why are these the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I've copied the needed files from both locations into one location to get inside the sandbox.

@@ -1,6 +1,6 @@
{ busybox }:

with import ./config.nix;
with import "${builtins.getEnv "_NIX_TEST_BUILD_DIR"}/config.nix";
Copy link
Member

Choose a reason for hiding this comment

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

Could this be factored into a file?
I think this would be nice:

Suggested change
with import "${builtins.getEnv "_NIX_TEST_BUILD_DIR"}/config.nix";
with import ./config.nix;

+

tests/functional/config.nix:

import "${builtins.getEnv "_NIX_TEST_BUILD_DIR"}/config.nix"

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh hah! that's not bad :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh but let's do that after we get rid of the old build system, because it will probably break it when we have a config.nix.in and config.nix in the same dir.

tests/functional/common/source-either.sh Outdated Show resolved Hide resolved
This should make `_NIX_TEST_ACCEPT=1` work again, fixing NixOS#11369.

Progress on NixOS#2503
@Ericson2314 Ericson2314 force-pushed the no-copy-functional-tests branch from b3e845b to 9d2ed0a Compare November 3, 2024 22:06
@Ericson2314 Ericson2314 enabled auto-merge November 3, 2024 22:07
@Ericson2314 Ericson2314 merged commit dd5a50d into NixOS:master Nov 3, 2024
11 checks passed
@Ericson2314 Ericson2314 deleted the no-copy-functional-tests branch November 4, 2024 19:39
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Nov 4, 2024
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants