Skip to content

edmarketconnector: do not add word "installPhase" to args #427415

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
Merged
merged 1 commit into from
Aug 20, 2025
Merged

edmarketconnector: do not add word "installPhase" to args #427415

merged 1 commit into from
Aug 20, 2025

Conversation

elfenermarcell
Copy link
Contributor

The current wrapper mistakenly adds the word "installPhase" to the program's arguments. This doesn't seem to cause any issue, but is unnecessary.

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Jul 22, 2025
@nix-owners nix-owners bot requested a review from jiriks74 July 22, 2025 10:43
@jiriks74
Copy link
Contributor

Hi,
sorry for my silence, I don't have that much free time this summer. From a quick look on my phone I'd say that it needs to be escaped rather than removed as users may want to pass flags to EDMarketConnector.

I'll take a proper look on my laptop, hopefully by Tuesday.

@elfenermarcell
Copy link
Contributor Author

Actually it does need to be removed, because if we look inside the generated wrapper:
before:

exec "/nix/store/xxkigfh0cz72l5fxnxk11qlhk30pf5lf-python3-3.13.5-env/bin/python"  /nix/store/mxw321qmyzwy5iqc78fq6canf72n7l02-source/EDMarketConnector.py installPhase "$@"

with this PR:

exec "/nix/store/xxkigfh0cz72l5fxnxk11qlhk30pf5lf-python3-3.13.5-env/bin/python"  /nix/store/mxw321qmyzwy5iqc78fq6canf72n7l02-source/EDMarketConnector.py "$@"

The "$@" gets added automatically. (Though github's copy button covers it...)

@jiriks74
Copy link
Contributor

jiriks74 commented Aug 1, 2025

Then everything seems good. Like I said I need to look into it a bit more but what you say makes sense so I wouldn't expect any issues when I get to my laptop.

jiriks74
jiriks74 approved these changes Aug 11, 2025
View reviewed changes
Copy link
Contributor

@jiriks74 jiriks74 left a comment

Choose a reason for hiding this comment

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

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 427415
Commit: b8c806c701aa7b33744a7d12f2cebc210887f9c5

x86_64-linux

✅ 1 package built:
  • edmarketconnector

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Aug 11, 2025
@r-vdp r-vdp merged commit b894fb7 into NixOS:master Aug 20, 2025
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@jiriks74 jiriks74 jiriks74 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elfenermarcell @jiriks74 @r-vdp