-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
pythonPackages.confuse: init at 1.0.0 #71263
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And can you squash it all down to one commit for the contributing guidelines :)
@jonringer thank you for your reviews! did apply them, additionally added the correct license, and squashed the commits to one commit |
|
||
propagatedBuildInputs = [ pyyaml ]; | ||
|
||
doCheck = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it with checkInputs = [ tox ];
as this package requires tox
for testing multiple interpreters (see https://github.com/beetbox/confuse/blob/1619d3ac33d2959cd5f4aa9ad725935b790ad12f/setup.py#L84 and https://github.com/beetbox/confuse/blob/1619d3ac33d2959cd5f4aa9ad725935b790ad12f/tox.ini#L7). However, it fails the check phase with Error creating virtualenv.
.
I looked at other projects, which rely on tox
, but failed to get it to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tox is not what you want, instead you want the other test inputs like pytest or what ever they use, then run the test suite with those. Generally people add tox as a convience to their own workflows, but it's relevant in nixpkgs since a python interpreter is parameterized to begin with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you @jonringer for your valuable input! After fiddling with getting the tests running I found that this perticular version (1.0.0
) does not yet include test/__init__.py
in MANIFEST.in
. According to their corresponding issue (beetbox/confuse#54), they want to deliver this in the new version 1.0.1
, which is still in preparation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's common for some people to exclude tests when packaging, but if they have a github repo, then you can just pull source from the repo
26d65a7
to
500e9ef
Compare
can you switch the ordering of the commits? it won't evaluate if i checkout 500e9ef.
|
ffecf9c
to
6bdcb0d
Compare
Co-Authored-By: Jon <jonringer@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nixpkgs-review failed with:
builder for '/nix/store/r72jnp137hfglvgvj1ab2kk3vzkha3wg-python3.8-confuse-1.0.0.drv' failed with exit code 1; last 10 log lines:
- ['\\build\\source\\~\\AppData\\Roaming']
+ ['C:\\Users\\test\\AppData\\Roaming']
Name Stmts Miss Cover
--------------------------------
confuse.py 756 122 84%
----------------------------------------------------------------------
Ran 198 tests in 0.307s
FAILED (SKIP=4, failures=2)
cannot build derivation '/nix/store/yhhsw9i37b6zg51mzy88cy254nfh53fl-env.drv': 1 dependencies couldn't be built
[7 built (1 failed), 30 copied (7.7 MiB), 1.7 MiB DL]
error: build of '/nix/store/yhhsw9i37b6zg51mzy88cy254nfh53fl-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/71263
1 package failed to build:
python38Packages.confuse
5 packages built:
python27Packages.confuse python27Packages.nose-show-skipped python37Packages.confuse python37Packages.nose-show-skipped python38Packages.nose-show-skipped
Please disable the build for python 3.8.
Was superseded by #91789 |
Motivation for this change
Confuse is a configuration library for Python that uses YAML.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)