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

remctl: init at 3.17 #110336

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

remctl: init at 3.17 #110336

wants to merge 1 commit into from

Conversation

invokesus
Copy link
Contributor

@invokesus invokesus commented Jan 21, 2021 •
edited

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.

SuperSandro2000
SuperSandro2000 requested changes Jan 21, 2021
View reviewed changes
pkgs/development/python-modules/remctl/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/remctl/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-packages.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/remctl/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/remctl/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/remctl/default.nix Outdated Show resolved Hide resolved
pkgs/tools/networking/remctl/default.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 110336 run on x86_64-linux 1

6 packages built:
  • perl530Packages.NetRemctl
  • perl532Packages.NetRemctl
  • python37Packages.remctl
  • python38Packages.remctl
  • python39Packages.remctl
  • remctl

@invokesus
Copy link
Contributor Author

Made the changes. Thanks for the review!

@stale
Copy link

stale bot commented Jul 26, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 26, 2021
djanatyn added a commit to djanatyn/nixpkgs that referenced this pull request May 10, 2022
@djanatyn
remctl: fixes from NixOS#110336
0565222
@djanatyn
Copy link
Contributor

djanatyn commented May 10, 2022 •
edited

Hi @invokesus and @SuperSandro2000, I was looking at remctl with @violetbp.

I pushed djanatyn@0565222 (on top of previous commits) addressing some of the review comments - feel free to pull those changes in for this PR.

Can you try patching the dependency out and see if it still works?

The package builds successfully after removing pytestrunner from setup.py with a patch.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 10, 2022
@invokesus invokesus requested a review from zakame as a code owner May 11, 2022 05:50
@invokesus
Copy link
Contributor Author

Thanks for looking at this, @djanatyn and @violetbp.

I applied your patch, added you as Co-author, and force-pushed.

@djanatyn @invokesus
remctl: init at 3.17
0c52edc 
Co-authored-by: Jonathan Strickland <djanatyn@gmail.com>
SuperSandro2000
SuperSandro2000 reviewed May 11, 2022
View reviewed changes
pkgs/development/python-modules/remctl/no-setup-requires-pytestrunner.patch
"long_description": "\n".join(doclines[2:]),
"license": "MIT",
"install_requires": ['typing;python_version<"3.5"'],
- "setup_requires": ["pytest-runner"],
Copy link
Member

Choose a reason for hiding this comment

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

Please use subsituteInPlace postPatch instead so that we do not need to update the patch.

pkgs/tools/networking/remctl/default.nix

stdenv.mkDerivation rec {
pname = "remctl";
version = "3.17";
Copy link
Member

Choose a reason for hiding this comment

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

Can we get 3.18 from https://github.com/rra/remctl ? I can't seem to browse their git.

pkgs/tools/networking/remctl/default.nix
postInstall = ''
cp -R perl $perlsrc
cp -R tests/tap/perl/Test $perlsrc/t/lib
cp -R python $pythonsrc
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this at all. We can just use the python directory from source straight.

pkgs/development/python-modules/remctl/default.nix
Comment on lines +6 to +8
postPatch = ''
sed -i '/install_requires/d' setup.py
'';
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't remove this line.

pkgs/development/python-modules/remctl/default.nix
patches = [
./no-setup-requires-pytestrunner.patch
];
propagatedBuildInputs = lib.optionals (isPy27) [ typing ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
propagatedBuildInputs = lib.optionals (isPy27) [ typing ];
propagatedBuildInputs = lib.optionals (pythonOlder "3.5") [ typing ];

pkgs/development/python-modules/remctl/default.nix
Comment on lines +9 to +11
buildInputs = [ remctl ];
checkInputs = [ pytestCheckHook ];
patches = [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
buildInputs = [ remctl ];
checkInputs = [ pytestCheckHook ];
patches = [
buildInputs = [ remctl ];
checkInputs = [ pytestCheckHook ];
patches = [

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 12, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank marked this pull request as draft March 20, 2024 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 requested changes

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@stigtsp stigtsp Awaiting requested review from stigtsp stigtsp is a code owner

@zakame zakame Awaiting requested review from zakame zakame is a code owner

@dasJ dasJ Awaiting requested review from dasJ dasJ will be requested when the pull request is marked ready for review dasJ is a code owner

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@invokesus @SuperSandro2000 @djanatyn @wegank