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.

Sign up for GitHub

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

Jump to bottom

nixops: unbreak #115779

Merged
merged 6 commits into from Mar 11, 2021
Merged

nixops: unbreak #115779

merged 6 commits into from Mar 11, 2021

Conversation

dotlambda
Copy link
Member

Motivation for this change
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.

dotlambda
dotlambda commented Mar 10, 2021
View reviewed changes
pkgs/development/python-modules/dateparser/0.x.nix
pytestFlagsArray = [ "tests" ];

disabledTestPaths = [
"tests/test_dateparser_data_integrity.py" # ImportError: No module named ruamel.yaml
Copy link
Member Author

Choose a reason for hiding this comment

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

@jonringer Can you explain this? Maybe it's related to a4fc8c4.

Copy link
Contributor

Choose a reason for hiding this comment

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

probably a PEP420 related issue, where only one of the ruamel packages can be imported.

Copy link
Contributor

Choose a reason for hiding this comment

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

or I avoided having to add another dependency. I don't remember

Copy link
Member Author

Choose a reason for hiding this comment

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

So we should probably add pythonNamespace to all ruamel packages?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this predated it.

It's totally plausible I wanted to avoid a dependency on raumel.yaml

@dotlambda dotlambda mentioned this pull request Mar 10, 2021
Closed
10 tasks
sternenseemann
sternenseemann approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

LGTM, nixops builds again

jonringer
jonringer approved these changes Mar 10, 2021
View reviewed changes
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.

diff LGTM though

pkgs/development/python-modules/dateparser/0.x.nix
pytestFlagsArray = [ "tests" ];

disabledTestPaths = [
"tests/test_dateparser_data_integrity.py" # ImportError: No module named ruamel.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

probably a PEP420 related issue, where only one of the ruamel packages can be imported.

pkgs/development/python-modules/dateparser/0.x.nix
pytestFlagsArray = [ "tests" ];

disabledTestPaths = [
"tests/test_dateparser_data_integrity.py" # ImportError: No module named ruamel.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

or I avoided having to add another dependency. I don't remember

@dotlambda
Copy link
Member Author

Can this go to master?

@FRidh
Copy link
Member

FRidh commented Mar 10, 2021

To reduce the amount of rebuilds on master it would probably help not to include the dropped support for Python 2.7 commits there for now, and push those to staging instead.

@dotlambda dotlambda mentioned this pull request Mar 10, 2021
Merged
10 tasks
@adisbladis adisbladis mentioned this pull request Mar 11, 2021
Closed
10 tasks
@dotlambda
Copy link
Member Author

@adisbladis Why did you get rid of the commits disabling some packages on Python 2? I had removed all commits that cause mass rebuilds.

@adisbladis
Copy link
Member

adisbladis commented Mar 11, 2021 •
edited

@FRidh @dotlambda In the interest of getting this merged quickly I took some liberties and followed @FRidh's advice and extracted the commits that drops python27 into a separate PR targeting staging in #115903 and force pushed this one.

@adisbladis
Copy link
Member

@adisbladis Why did you get rid of the commits disabling some packages on Python 2? I had removed all commits that cause mass rebuilds.

Sorry about that, I must have misread the Github labels. I thought the mass rebuild labels were still valid.

@adisbladis adisbladis merged commit 0213d5f into NixOS:master Mar 11, 2021
@dotlambda dotlambda deleted the nixops-unbreak branch March 11, 2021 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer approved these changes

@sternenseemann sternenseemann sternenseemann approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@adisbladis adisbladis Awaiting requested review from adisbladis

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dotlambda @FRidh @adisbladis @sternenseemann @jonringer