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

pythonPackages.{aadict,,globre,pxml}: init at 0.2.3, 0.1.5, 0.2.13 respectively #84973

Merged
merged 4 commits into from Jul 14, 2020

Conversation

glittershark
Copy link
Member

@glittershark glittershark commented Apr 11, 2020

Motivation for this change

Needed by asset (coming soon) which is needed by passwordmeter (coming soon) which I need in my application.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

@progval
Copy link
Member

progval commented Apr 12, 2020

I get this error when building with nixpkgs-review pr 84973:

$ nix build --no-link --keep-going --option build-use-sandbox relaxed -f /home/val/.cache/nixpkgs-review/pr-84973-2/build.nix
warning: ignoring the user-specified setting 'sandbox', because it is a restricted setting and you are not a trusted user
builder for '/nix/store/5myfvvn8896z5qqlspxjhkskw9yw3kgp-python3.8-pxml-0.2.13.drv' failed with exit code 1; last 10 log lines:
    File "/build/pxml-0.2.13/pxml/test.py", line 89, in test_canonicalAttributeOrder
      self.assertEqual(out.getvalue(), chk)
  AssertionError: b'<?x[28 chars]TF-8"?>\n<root>\n  <zig a="1" c="2" b="3">foo</zig>\n</root>\n' != b'<?x[28 chars]TF-8"?>\n<root>\n  <zig a="1" b="3" c="2">foo</zig>\n</root>\n'
  
  ----------------------------------------------------------------------
  Ran 1 test in 0.012s
  
  FAILED (failures=1)
  Test failed: <unittest.runner.TextTestResult run=1 errors=0 failures=1>
  error: Test failed: <unittest.runner.TextTestResult run=1 errors=0 failures=1>
cannot build derivation '/nix/store/a87fbh6nfm25ksayjbfvvwrpmg4zfp3l-env.drv': 1 dependencies couldn't be built

@glittershark
Copy link
Member Author

probably 3.8 specific. I'll mark that package broken on 3.8 if so

@glittershark glittershark changed the title pythonPackages.aadict: init at 0.2.3 pythonPackages.{aadict,,globre,pxml}: init at 0.2.3, 0.1.5, 0.2.13 respectively Apr 12, 2020
@glittershark glittershark force-pushed the init-aadict branch 2 times, most recently from f4a6e18 to e1529b4 Compare April 12, 2020 21:10
@glittershark
Copy link
Member Author

Yep, pxml fails on python >= 3.8. Marked as disabled for that version onwards.

@glittershark glittershark force-pushed the init-aadict branch 2 times, most recently from 2a5254c to 521365d Compare April 13, 2020 01:12
Copy link
Contributor

@bhipple bhipple left a comment

Choose a reason for hiding this comment

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

It'd be much nicer if these all built on python 3.7 and 3.8, but at least Nix will skip them and their dependencies, so it won't cause build failures in Hydra. Ideally upstream gets their act together!

In the meantime, the actual packaging expressions look good to me.

@bhipple
Copy link
Contributor

bhipple commented May 8, 2020

Result of nixpkgs-review pr 84973 1

1 package failed to build:
- python27Packages.pxml
5 packages built:
- python27Packages.aadict
- python27Packages.globre
- python37Packages.aadict
- python37Packages.pxml
- python38Packages.aadict

Copy link
Contributor

@bhipple bhipple left a comment

Choose a reason for hiding this comment

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

Nevermind, still doesn't pass nixpkgs-review pr 84973

@glittershark
Copy link
Member Author

@bhipple should be fixed now - also figured out the way to exclude tests in nosetests which simplifies pxml quite a bit too

@glittershark glittershark requested a review from bhipple May 8, 2020 15:44
Disabled this on python >= 3.7, since the test suite fails, though I'm
sure if someone else wants it on a new python version the test suite
could be fixed.
@glittershark
Copy link
Member Author

/marvin opt-in

@marvin-mk2 marvin-mk2 bot added the marvin label Jun 16, 2020
@marvin-mk2
Copy link

marvin-mk2 bot commented Jun 16, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. The stages are

  • needs_review, if the author considers this PR ready
  • needs_work if the PR in its current form is not ready yet. Maybe the reviewer requested changes, there is an ongoing discussion or you are waiting for upstream feedback.
  • needs_merge can be set by reviewers who do not have merge permission but would merge this PR if they could.

Anybody can switch the current status with a comment of the form /status <new_status_here>.

Feedback and contributions to this bot are appreciated.

@glittershark
Copy link
Member Author

/status needs_review

@symphorien
Copy link
Member

pxml upstream does not look very lively, it's not building with python3.8 which will sooner or later be the default, and it has no reverse dependencies in nixpkgs... I wonder if integrating this module to nixpkgs is the best course of action.
Otherwise, the nix expression looks good and nix-review builds.

@glittershark
Copy link
Member Author

It's in my transitive dependency chain because of passwordmeter -> asset -> pxml, fwiw.

@glittershark
Copy link
Member Author

feels kinda bad to have pip packages that exist but can't be installed from nixpkgs, basically always

@marvin-mk2 marvin-mk2 bot requested a review from ryantm July 14, 2020 02:49
@ryantm ryantm merged commit 6faba43 into NixOS:master Jul 14, 2020
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

6 participants