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

pytestdjango: init at 2.9.1 #14684

Merged
merged 3 commits into from
May 9, 2016
Merged

pytestdjango: init at 2.9.1 #14684

merged 3 commits into from
May 9, 2016

Conversation

edugomez
Copy link
Contributor

Things done
  • Tested using sandboxing (nix-build --option build-use-chroot true or nix.useChroot on NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@joachifm joachifm added 6.topic: python 8.has: package (new) This PR adds a new package labels Apr 15, 2016

# doing this to allow depending packages to find
# pytest's binaries
pytest = self.pytest;
Copy link
Member

@FRidh FRidh Apr 23, 2016

Choose a reason for hiding this comment

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

I don't see how this could be useful.
Depending packages will be able to find pytest because you added it already to propagatedBuildInputs.

Copy link
Contributor

@risicle risicle Apr 25, 2016

Choose a reason for hiding this comment

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

@FRidh So how would a depending package be able to e.g. check the version of pytest that pytestdjango is built against?

Copy link
Member

Choose a reason for hiding this comment

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

Nix builds up a graph of all dependencies. You've stated that pytest is a dependency of pytestdjango.
If another package now depends on pytestdjango, it can follow the graph and get that specific pytest version.

With Python, in order for packages to be visible, they need to be added to PYTHONPATH. If you now create an environment with multiple versions of pytest, e.g. mixing the 2.7 and 3.5 versions (note, they are different nodes in the Nix graph, since pkgs.python27Packages != pkgs.python35Packages), then they are both added to PYTHONPATH. This is a problem. Which version should Python now import? This is actually a general problem with Python and the reason why virtualenv is used.

You gave the buildPythonPackage, Python definitions, the keyword argument pytest = self.pytest, but this parameter doesn't exist and is ignored.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps what you are looking for is like for example numpy.blas where in the function that builds numpy we have passthru.blas = blas`.

https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/numpy.nix

That way, if another package needs that version of pytest as a buildInput, they can just add pytestdjango.pytest.

I don't think that's necessary here since in python-packages.nix we have only one pytest. The reason it is done for numpy.blas is that in all-packages.nix we have different BLAS implementations, and in case one of the Python packages uses a different one than the other we could get segmentation faults.

Copy link
Contributor

Choose a reason for hiding this comment

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

This particular case is to allow a depending package to directly reference the py.test binary so it can create a wrapper - it just seemed slightly neater for the depending package to only require pytestdjango as an argument rather than pytest and pytestdjango, while having the bonus of ensuring there isn't any version mixup.

Copy link
Member

Choose a reason for hiding this comment

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

But you've already added pytest to propagatedBuildInputs, so any package depending on pytestdjango doesn't have to add pytest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that works from a runtime perspective, but if a depending package is able to grab the specific pytest's outpath, it enables it to e.g. build a wrapper shell script which directly references the specific pytest's binary rather than having to rely on runtime PATH.

Copy link
Member

Choose a reason for hiding this comment

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

That is a good reason. But does that work like this? Could you show an expression how you depend on pytestdjango? As far as I know you can only refer pytest if you add it as an attribute to passthru. Right now it is an extra argument to buildPythonPackage which is not used and thus ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so here's somewhere I use it - in a django project "package", which depends on its main code, xyzCode, which itself depends on a whole host of things, which it then itself exposes as attrs. One of those is pytestdjango. Then I can do:

installPhase = ''
  ...
  cp ${xyzCode.pytestdjango.pytest}/bin/.py.test-wrapped $out/bin/py.test
  ...
'';

Copying the actual py.test binary into the project outpath causes wrapPythonPrograms to re-wrap it with the project's PYTHONPATH etc. values, meaning that we've created a py.test command which, whatever context it runs in, will always be run with the project's standard paths setup. None of these odd but it works one way when I'm running this from manage.py and another when invoked from py.test problems.

Copy link
Contributor

@risicle risicle May 11, 2016

Choose a reason for hiding this comment

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

Actually the above example is far more complex than it has to be... all you have to consider is I might want to do e.g.

someCommand = writeScript "some-command" ''
  ...
  ${xyzCode.pytestdjango.pytest}
  ...
'';

to create some very package-specific script and know that it's going to refer to the exact binary it's built against, not swayed by whatever the user happens to have in their $PATH at runtime.

@FRidh FRidh added the 2.status: work-in-progress This PR isn't done label Apr 23, 2016
@zimbatm
Copy link
Member

zimbatm commented May 5, 2016

Hi, can you update the src to use the new mirror://pypi/ ? Adding pytest = self.pytest; is debatable but I don't think should be a blocker for merge.

@zimbatm zimbatm merged commit 0290405 into NixOS:master May 9, 2016
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: work-in-progress This PR isn't done 6.topic: python 8.has: package (new) This PR adds a new package 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants