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

Packages for the python coreapi client #66391

Merged
merged 4 commits into from Jan 21, 2020
Merged

Packages for the python coreapi client #66391

merged 4 commits into from Jan 21, 2020

Conversation

ghost
Copy link

@ghost ghost commented Aug 9, 2019

Motivation for this change

I want to package my application with Nix and I need the coreapi package.

Things done

The coreapi python package depends on 2 packages that were not in Nix. I've packaged those as well. One of them had to be forked and updated. I've added myself as a maintainer to them.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@catern
Copy link
Contributor

catern commented Aug 9, 2019

What are the changes to your fork of pythonPackages.coreschema? Typically we would keep those patches in the Nixpkgs repo if they are small.

The packages look good overall.

@ghost
Copy link
Author

ghost commented Aug 9, 2019

The fork deletes two files which aren't used but cause build errors. The errors occur when building the python3x version of the package, and without the files, builds succeed on all python versions (from what I can tell).

@catern
Copy link
Contributor

catern commented Aug 9, 2019

That seems to me like something which could be done as part of the Nix package, maybe good to do that directly in the package so that we can use upstream directly?

Although I see the upstream is archived, so maybe that's not useful; is there a fork of this package that is still actively developed/maintained?

@ghost
Copy link
Author

ghost commented Aug 9, 2019

There are no forks that are maintained. All of them are behind or even with master. Should I patch up stream?

@ghost
Copy link
Author

ghost commented Aug 9, 2019

I decided to patch upstream. The dependencies are still building, but I'm 99.99999999% confident it's fine.

EDIT: build passes without a problem

@catern
Copy link
Contributor

catern commented Aug 9, 2019

Yeah, that looks better, thanks for the change. This PR looks good to me now.

@ghost
Copy link
Author

ghost commented Aug 10, 2019

Thanks for the review!

@risicle
Copy link
Contributor

risicle commented Aug 11, 2019

To get the tests executing for the coreapi package:

  src = fetchFromGitHub {
    repo = "python-client";
    owner = "core-api";
    rev = version;
    sha256 = "1c6chm3q3hyn8fmjv23qgc79ai1kr3xvrrkp4clbqkssn10k7mcw";
  };

The pypi package doesn't include the tests - they're present in the github archive though.

  checkInputs = [ pytest ];
  checkPhase = ''
    mv coreapi coreapi.hidden
    pytest tests
  '';

WFM macos 10.13

@risicle
Copy link
Contributor

risicle commented Aug 11, 2019

I'm a little confused over the necessity of rming the files from coreschema - it seems to remove functionality.

The tests for coreschema can be enabled in a similar way as above:

  src = fetchFromGitHub {
    repo = "python-coreschema";
    owner = "core-api";
    rev = version;
    sha256 = "027pc753mkgbb3r1v1x7dsdaarq93drx0f79ppvw9pfkcjcq6wb1";
  };

and

  checkInputs = [ pytest ];
  checkPhase = ''
    mv coreschema coreschema.hidden
    pytest tests
  '';

However these naturally include tests for the modules you've rmed, which of course fail because they can't find the files. But if you leave those files in, the tests pass 100% for me, and what's more the tests for coreapi, depending on it, pass 100%. So this would seem to indicate that it's functioning properly (this is python 3.7). What exactly is the problem you're having with these files present?

@risicle
Copy link
Contributor

risicle commented Aug 11, 2019

And for itypes tests:

  src = fetchFromGitHub {
    repo = pname;
    owner = "tomchristie";
    rev = version;
    sha256 = "0zkhn16wpslkxkq77dqw5rxa28nrchcb6nd3vgnxv91p4skyfm62";
  };

and

  checkInputs = [ pytest ];
  checkPhase = ''
    mv itypes.py itypes.py.hidden
    pytest tests.py
  '';

@ghost
Copy link
Author

ghost commented Aug 12, 2019

@risicle thanks for the review!

I've incorporated your changes, but I'm wondering, what's the purpose of moving the python module to .hidden?

The error was because in coreschema/encodings/corejson there are no imports defined. This caused an error about the objects not being found. It seems to have disappeared after using your way to fetch from Github.

@risicle
Copy link
Contributor

risicle commented Aug 12, 2019

I've incorporated your changes, but I'm wondering, what's the purpose of moving the python module to .hidden?

Oh, it's to "prove" that the tests are being run against the installed module, rather than the directory-local source dir.

It seems to have disappeared after using your way to fetch from Github.

Ah right yeah, sometimes people screw up the pypi packaging of a tarball, quite possible that's what's happened here.

@ghost
Copy link
Author

ghost commented Aug 12, 2019

Is there a problem that I added an additional package that depends on the ones in the PR?

I followed the same conventions as the previous ones. The new package is drf-yasg which generates API documentation from django-rest-framework schemas.

I tried to fetch the source from GitHub to run the tests, but I couldn't get past the modules attempting to access the django.conf.settings module. In case anyone wants to try. I had to do a little hack to get the GitHub build to work on python3x.

{
  stdenv,
  buildPythonPackage,
  fetchFromGitHub,
  inflection,
  ruamel_yaml,
  setuptools_scm,
  six,
  coreapi,
  djangorestframework,
  pytest,
  datadiff,
}:

buildPythonPackage rec {
  pname = "drf-yasg";
  version = "1.16.1";

  # setuptools_scm error on python3x
  # https://github.com/python-cmd2/cmd2/pull/681#issuecomment-493833472
  SETUPTOOLS_SCM_PRETEND_VERSION="0.9.13";

  src = fetchFromGitHub {
    repo = pname;
    owner = "axnsan12";
    rev = version;
    sha256 = "1kn1ys9d6lsyh5c2bmrdf0hx7gqm2ds61zipvkc3j4jxzdvb668b";
  };

  propagatedBuildInputs = [
    six
    inflection
    ruamel_yaml
    setuptools_scm
    coreapi
    djangorestframework
  ];

  checkInputs = [ pytest datadiff ];
  checkPhase = ''
    pytest tests
  '';

  meta = with stdenv.lib; {
    description = "Generation of Swagger/OpenAPI schemas for Django REST Framework";
    homepage = https://github.com/axnsan12/drf-yasg;
    maintainers = with maintainers; [ ivegotasthma ];
    license = licenses.bsd3;
  };
}

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM, just one minor suggestion (doesn't change closure size so :/ )

pkgs/development/python-modules/drf-yasg/default.nix Outdated Show resolved Hide resolved
@jonringer
Copy link
Contributor

Is there a problem that I added an additional package that depends on the ones in the PR?

No one will likely care much as long as you're a maintainer. And it's related to the others, should be fine.

@FRidh
Copy link
Member

FRidh commented Aug 13, 2019

Oh, it's to "prove" that the tests are being run against the installed module, rather than the directory-local source dir.

Please change into the tests directory then instead. That way the source won't get on sys.path.

@risicle
Copy link
Contributor

risicle commented Aug 13, 2019

Please change into the tests directory then instead.

This doesn't always work - sometimes tests expect the cwd to be the repo root for some reason.

@ghost
Copy link
Author

ghost commented Aug 14, 2019

Please change into the tests directory then instead. That way the source won't get on sys.path.

I can't find an example how to do that. Is it just changing the directory the tests run from?

EDIT: itypes has tests in the root folder, what should I do in that case? I think the current solution works pretty well.

@jonringer
Copy link
Contributor

checkPhase = ''
  cd test/
  pytest
'';

you're over thinking it :)

@ghost
Copy link
Author

ghost commented Aug 14, 2019

Thanks a lot, fixed :)

@ghost
Copy link
Author

ghost commented Aug 22, 2019

I think this is ready to be merged, is there something left @FRidh ?

@risicle risicle mentioned this pull request Aug 23, 2019
10 tasks
@lsix lsix merged commit e0fa73d into NixOS:master Jan 21, 2020
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Jan 21, 2020
…kage-coreapi

Packages for the python coreapi client

(cherry picked from commit e0fa73d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants