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

iputils: 20180629 -> 20190324 #58215

Merged
merged 1 commit into from Mar 27, 2019
Merged

iputils: 20180629 -> 20190324 #58215

merged 1 commit into from Mar 27, 2019

Conversation

primeos
Copy link
Member

@primeos primeos commented Mar 24, 2019 •
edited

Important changes:

  • Two files where removed in 0:
    • bin/ipg
    • share/man/man8/pg3.8.gz
  • The build system changed from Autotools to Meson.
  • "It is probable that within in many many changes a regression may have
    slipped in" 1
Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
    • Actually got smaller:
/nix/store/22nfcbw7n7yd87yql537pma23h47bfzz-iputils-20180629       42322272
/nix/store/5ks8y96py4scqgq6i43knvjhai0b8mzp-iputils-20190324       41023192
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dtzWill dtzWill mentioned this pull request Mar 24, 2019
Closed
10 tasks
@dtzWill
Copy link
Member

dtzWill commented Mar 24, 2019

This likely will need a fix along the lines of the patch on #58224 in order to work w/musl. I'll ask upstream but if we could avoid breaking in the meantime that'd be great :).

@dtzWill
Copy link
Member

dtzWill commented Mar 24, 2019

dtzWill@0d29e98
dtzWill@edc5239

@dtzWill
Copy link
Member

dtzWill commented Mar 24, 2019

Opened upstream issue: iputils/iputils#168

@primeos
Copy link
Member Author

primeos commented Mar 25, 2019 •
edited

This likely will need a fix along the lines of the patch on #58224 in order to work w/musl. I'll ask upstream but if we could avoid breaking in the meantime that'd be great :).

Yes, I've included your patch for now - thanks for opening the upstream issue :)

I've also put lipcap in nativeBuildInputs because of the following:

setcap = find_program('setcap', '/usr/sbin/setcap', '/sbin/setcap', required : false)

which is not required but the build will fail when setcap.path() is an empty string.
Another alternative would be to add yet another patch (due to the sandbox setcap will fail anyway).

What do you think of this?

Edit: Saw that you've also added libcap to nativeBuildInputs in your PR.

@primeos primeos force-pushed the iputils branch 2 times, most recently from 49e53d8 to 3b079fd Compare March 25, 2019 15:33
@primeos
Copy link
Member Author

primeos commented Mar 25, 2019

Do we want the following new files by default?:

│ +etc
│ +etc/systemd
│ +etc/systemd/system
│ +etc/systemd/system/ninfod.service
│ +etc/systemd/system/rarpd@.service
│ +etc/systemd/system/rdisc@.service
│ +sbin
│ +share/locale
│ +share/locale/ja
│ +share/locale/ja/LC_MESSAGES
│ +share/locale/ja/LC_MESSAGES/iputils.mo

Possible alternatives: Don't build them by default or move them to other outputs.

jtojnar
jtojnar reviewed Mar 25, 2019
View reviewed changes
pkgs/os-specific/linux/iputils/default.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/iputils/default.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/iputils/default.nix Outdated Show resolved Hide resolved
@jtojnar
Copy link
Contributor

jtojnar commented Mar 25, 2019

What is the problem with Japanese?

@primeos
Copy link
Member Author

primeos commented Mar 25, 2019

What is the problem with Japanese?

Well nothing actually. I guess I was just too concerned about extra files that might not be that necessary.

@primeos primeos requested review from jtojnar and dtzWill March 25, 2019 22:25
@jtojnar
Copy link
Contributor

jtojnar commented Mar 25, 2019

Looks about right. We might want to fetch iputils/iputils@c503834 to be able to use docbook_xml_ns but if it works, adding just a TODO comment to switch to it on next release should be enough.

@primeos @dtzWill
iputils: 20180629 -> 20190324
b6e50e4 
Important changes:
- Two files where removed in [0]:
  - bin/ipg
  - share/man/man8/pg3.8.gz
- The build system changed from Autotools to Meson.
- New: systemd units and Japanese translations
- "It is probable that within in many many changes a regression may have
  slipped in" [1]

[0]: iputils/iputils@45a1d39
[1]: https://github.com/iputils/iputils/releases/tag/s20190324

Co-authored-by: Will Dietz <github@wdtz.org>
@primeos
Copy link
Member Author

primeos commented Mar 26, 2019

We might want to fetch iputils/iputils@c503834 to be able to use docbook_xml_ns but if it works, adding just a TODO comment to switch to it on next release should be enough.

Ok, yeah might be better to change it before I forget about it. The man pages did not change according to diffoscope so everything should still be fine :)

Do the dependencies look ok now?

@jtojnar
Copy link
Contributor

jtojnar commented Mar 26, 2019

Looks good.

@primeos primeos merged commit c4eff69 into NixOS:master Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lheckemann lheckemann Awaiting requested review from lheckemann

@jtojnar jtojnar Awaiting requested review from jtojnar

@dtzWill dtzWill Awaiting requested review from dtzWill

Assignees
No one assigned
Labels
8.has: clean-up 10.rebuild-darwin: 0 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+ 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@primeos @dtzWill @jtojnar @GrahamcOfBorg