Skip to content

Test store paths, with property tests, fix bug#7639

Merged
roberth merged 3 commits intoNixOS:masterfrom
obsidiansystems:output-names
Jan 23, 2023
Merged

Test store paths, with property tests, fix bug#7639
roberth merged 3 commits intoNixOS:masterfrom
obsidiansystems:output-names

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jan 19, 2023 •
edited by roberth
Loading

Motivation

We want mores tests in general. We also have separate code parsing store paths names (when parsing StorePath and when parsing OutputsSpec) that want to keep in sync.

The property test in fact found a bug: we were excluding numbers!

The commits are broken up so that reviewing commit by commit should make for easy reading.

Context

#7633 is an issue to start making property tests for Nix (a la QuickCheck). This kicks off that process, adding rapidcheck as a dependency, and using it for these new tests.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests
  • documentation in the manual
    • link to rapidcheck
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes
    • we don't have a habit of mentioning fixes or documenting patch releases properly yet; postponed

@Ericson2314 Ericson2314 changed the title Test store paths Test store paths, with property tests Jan 19, 2023
@Ericson2314 Ericson2314 requested a review from roberth January 19, 2023 13:33
@Ericson2314 Ericson2314 mentioned this pull request Jan 19, 2023
Closed
Ericson2314
Ericson2314 commented Jan 19, 2023
View reviewed changes
Ericson2314
Ericson2314 commented Jan 19, 2023
View reviewed changes
@Ericson2314 Ericson2314 marked this pull request as ready for review January 19, 2023 13:39
@Ericson2314 Ericson2314 force-pushed the output-names branch 2 times, most recently from fbf181d to 13c355f Compare January 19, 2023 14:30
Ericson2314
Ericson2314 commented Jan 19, 2023
View reviewed changes
cole-h
cole-h reviewed Jan 19, 2023
View reviewed changes
roberth
roberth reviewed Jan 19, 2023
View reviewed changes
@Ericson2314 Ericson2314 requested a review from roberth January 20, 2023 13:04
@Ericson2314 Ericson2314 changed the title Test store paths, with property tests Test store paths, with property tests, fix bug Jan 20, 2023
roberth
roberth reviewed Jan 23, 2023
View reviewed changes
Ericson2314 and others added 3 commits January 23, 2023 07:05
@Ericson2314 @cole-h
Add rapidcheck dependency for testing
7fe308c 
Property tests are great!

Co-authored-by: Cole Helbling <[email protected]>
@Ericson2314
Better-scope Store forward declarations
6853953
@Ericson2314
Test store paths, with property tests
018e257 
The property test in fact found a bug: we were excluding numbers!
@roberth roberth merged commit a58e9c3 into NixOS:master Jan 23, 2023
@github-actions github-actions bot mentioned this pull request Jan 23, 2023
Merged
@github-actions
Copy link

github-actions bot commented Jan 23, 2023

Successfully created backport PR #7664 for 2.13-maintenance.

@Ericson2314 Ericson2314 deleted the output-names branch January 23, 2023 14:31
@Ericson2314 Ericson2314 mentioned this pull request Jan 24, 2023
Open
@yorickvP
Copy link
Contributor

yorickvP commented Jan 30, 2023

This PR breaks buliding in nix develop with at least clangStdenv, and possibly everywhere.

./configure $configureFlags --prefix=$(pwd)/outputs/out now expands to ./configure CXXFLAGS=-I/nix/store/l6z73w3nqkjk5rq6913sf9vigx1vyv05-rapidcheck-unstable-2020-12-19/extras/gtest/include --with-boost=/nix/store/lf8ghiy89rfhqqyh9j98270afwjqzyrb-boost-1.79.0/lib --with-sandbox-shell=/nix/store/7b943a2k4amjmam6dnwnxnj8qbba9lbq-busybox-static-x86_64-unknown-linux-musl-1.35.0/bin/busybox LDFLAGS=-fuse-ld=gold --prefix=/home/yorick/tweag/nix/outputs/out

Which interprets everything as CXXFLAGS. The configure script then errors out about boost, but looking in config.status has a whole bunch of clang-11: error: no such file or directory: 'LDFLAGS=-fuse-ld=gold'

Reordering to put the new CXXFLAGS last makes it work. This is the documented behavior for ./configure

Usage: ./configure [OPTION]... [VAR=VALUE]...

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 30, 2023

@yorickvP I am seeing

  $ ./configure ... CXXFLAGS=-I/nix/store/l6
z73w3nqkjk5rq6913sf9vigx1vyv05-rapidcheck-unstable-2020-12-19/extras/gtest/include --with-
boost=/nix/store/lf8ghiy89rfhqqyh9j98270afwjqzyrb-boost-1.79.0/lib --with-sandbox-shell=/n
ix/store/7b943a2k4amjmam6dnwnxnj8qbba9lbq-busybox-static-x86_64-unknown-linux-musl-1.35.0/
bin/busybox LDFLAGS=-fuse-ld=gold

ac_cv_env_CXXFLAGS_set=set
ac_cv_env_CXXFLAGS_value=-I/nix/store/l6z73w3nqkjk5rq6913sf9vigx1vyv05-rapidcheck-unstable-2020-12-19/extras/gtest/include

in a local config.log, which is to say CXXFLAGS=... is not the last configure argument and yet not too much got gobbled up, so I am not sure what is going on.

@yorickvP
Copy link
Contributor

yorickvP commented Jan 31, 2023

@Ericson2314 you're right.
It seems that fish was passing $configureFlags as a single argument to ./configure, which worked before but not anymore.
I've changed my configure command to ./configure (string split ' ' $configureFlags) --prefix=$(pwd)/outputs/out to work around it.
Not sure how common this case will be (direnv+fish).

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 31, 2023

@yorickvP Ah yes. I use fish too but not in my Nix development shell so I never noticed that. Glad it's working for you now!

@nixos-discourse
Copy link

nixos-discourse commented Feb 13, 2023

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-01-23-nix-team-meeting-minutes-26/25433/1

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

Reviewers

@fricklerhandwerk fricklerhandwerk Awaiting requested review from fricklerhandwerk

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

2 more reviewers

@cole-h cole-h cole-h left review comments

@roberth roberth roberth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@roberth roberth

Labels

None yet

Projects

Nix team
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Ericson2314 @yorickvP @nixos-discourse @roberth @cole-h @thufschmitt