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

amazon-ec2-utils: Merge with / replace ec2-utils #110930

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

amazon-ec2-utils: Merge with / replace ec2-utils #110930

wants to merge 1 commit into from

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Jan 27, 2021

Motivation for this change

In #108804 aws-ec2-utils was packaged, not noticing that a much
older version of the same project was already packaged in nixpkgs
before as ec2-utils.

This commit merges the two:

  • Amazon recently published the full source repo on GitHub and
    renamed the project (see amazon-ec2-utils.spec changelog):

    Rename the project to amazon-ec2-utils

  • The old one was extracted from a RPM, the new one is built from source.
  • The new one is the more recent version 1.3.
  • In the new one we hadn't installed udev rules yet; in the old we did.
    The merged version has both.
  • Keeping virtualization/ as the containing folder (from the old).
  • Maintainers merged.
  • installCheck added to test execution.
  • Alias added for the old name.

CC @thefloweringash @ketzacoatl

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.

@nh2
amazon-ec2-utils: Merge with / replace ec2-utils
f39ef5d 
In NixOS#108804 `aws-ec2-utils` was packaged, not noticing that a much
older version of the same project was already packaged in nixpkgs
before as `ec2-utils`.

This commit merges the two:

* Amazon recently published the full source repo on GitHub and
  renamed the project (see `amazon-ec2-utils.spec` changelog):
  > Rename the project to amazon-ec2-utils
* The old one was extracted from a RPM, the new one is built from source.
* The new one is the more recent version 1.3.
* In the new one we hadn't installed udev rules yet; in the old we did.
  The merged version has both.
* Maintainers merged.
* `installCheck` added to test execution.
* Alias added for the old name.
@nh2 nh2 force-pushed the amazon-ec2-utils-merge-ec2-utils branch from 3a095e6 to f39ef5d Compare January 27, 2021 14:44
@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 110930 run on x86_64-linux 1

1 package built:
  • amazon-ec2-utils

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

amazon-ec2-utils:

Please consider this feature to be alpha.

A substituteInPlace with an unmatched pattern got detected:

substituteStream(): WARNING: pattern '/sbin' doesn't match anything in file '/nix/store/5rgi8b04nzd02mf93h5iymivbh0krhkc-amazon-ec2-utils-1.3/etc/udev/rules.d/60-cdrom_id.rules'

Please check the offending substituteInPlace for typos or changes in source.

@nh2
Copy link
Contributor Author

nh2 commented Jan 27, 2021

A substituteInPlace with an unmatched pattern got detected:

@SuperSandro2000 That is very useful, how does that work?

pattern '/sbin' doesn't match anything

Something in nixpkgs seems to move sbin/ contents to bin.

Should we replace the use of sbin generally in installPhase?

@SuperSandro2000
Copy link
Member

how does that work?

It greps the log for the occurrence of that error. See https://github.com/SuperSandro2000/nixpkgs-review-checks/blob/master/nixpkgs-review-checks#L80 for the exact code.

Something in nixpkgs seems to move sbin/ contents to bin.

I think because the difference is very blurred in nixpkgs.

Should we replace the use of sbin generally in installPhase?

I am not sure.

ketzacoatl
ketzacoatl approved these changes Jan 27, 2021
View reviewed changes
Copy link
Contributor

@ketzacoatl ketzacoatl left a comment

Choose a reason for hiding this comment

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

This is great, thank you for merging them together. While I cannot review the nix as well, the notes on what was merged sounds good to me.

@tomberek
Copy link
Contributor

tomberek commented Mar 3, 2021

Requires re-base.

@ketzacoatl
Copy link
Contributor

@nh2 could I help, or should I leave it to you?

@stale
Copy link

stale bot commented Sep 3, 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 Sep 3, 2021
@thefloweringash
Copy link
Member

Not stale, still valuable cleanup.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 3, 2021
@ketzacoatl
Copy link
Contributor

@SuperSandro2000 / @nh2 / @tomberek,

I'd like to complete this to get it merged.

Aside from rebasing, are there any updates needed for the bin/sbin topic discussed above?

SuperSandro2000
SuperSandro2000 requested changes Sep 21, 2021
View reviewed changes
pkgs/tools/virtualization/amazon-ec2-utils/default.nix
};

buildInputs = [
python3
Copy link
Member

Choose a reason for hiding this comment

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

You are sure this is not nativeBuildInputs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, ebsnvme-id is a python script, so python is needed at runtime (and not at build time).

pkgs/tools/virtualization/amazon-ec2-utils/default.nix
];

propagatedBuildInputs = [
curl
Copy link
Member

Choose a reason for hiding this comment

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

Why not use a wrapper?

pkgs/tools/virtualization/amazon-ec2-utils/default.nix
installPhase = ''
install -Dm755 ec2-metadata --target-directory $out/bin/

install -Dm755 --target-directory $out/sbin/ ebsnvme-id
Copy link
Member

Choose a reason for hiding this comment

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

NixOS does not use a sbin directory.

@tomprince tomprince mentioned this pull request Feb 1, 2022
Merged
13 tasks
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 1, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 14, 2022
@ketzacoatl
Copy link
Contributor

See also #228881 for another update to the amazon-ec2-utils package.

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

@tomprince tomprince tomprince left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 requested changes

@ketzacoatl ketzacoatl ketzacoatl approved these changes

@thefloweringash thefloweringash Awaiting requested review from thefloweringash

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
2.status: merge conflict 8.has: clean-up 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@nh2 @SuperSandro2000 @tomberek @ketzacoatl @thefloweringash @tomprince @wegank @bobby285271