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

udpt: 2017-09-27 -> 3.1.1 #93586

Merged
merged 2 commits into from
Jul 23, 2020
Merged

udpt: 2017-09-27 -> 3.1.1 #93586

merged 2 commits into from
Jul 23, 2020

Conversation

makefu
Copy link
Contributor

@makefu makefu commented Jul 21, 2020 •
edited
Loading

Motivation for this change

udpt did a complete rewrite from cpp to rust.
This Commit uses rust with cargo to build the package.
The Configuration format changed form .conf to .toml and
the binary changed from udpt to udpt-rs

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)
  • [NA] 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 - update release notes
  • Fits CONTRIBUTING.md.

@makefu makefu marked this pull request as ready for review July 21, 2020 11:40
@makefu makefu requested a review from flokli July 21, 2020 11:41
flokli
flokli reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

We usually drop /usr from the locations.

And generally, if this is an example config, maybe $out/etc/udpt/udpt.toml or $out/share/udpt/udpt.toml are more appropriate?

nixos/doc/manual/release-notes/rl-2009.xml Outdated
<literal>udpt</literal> experienced a complete rewrite from C++ to rust. The configuration format changed from ini to toml.
The new configuration documentation can be found at
<link xlink:href="https://naim94a.github.io/udpt/config.html">the official website</link> and example
configuration is packaged in <literal>${udpt}/usr/lib/udpt/udpt.toml</literal>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
configuration is packaged in <literal>${udpt}/usr/lib/udpt/udpt.toml</literal>.
configuration is packaged in <literal>${udpt}/lib/udpt/udpt.toml</literal>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, is there a convention which files go where? Upstream uses /usr/src which didn't really convince me as well :)

pkgs/servers/udpt/default.nix Outdated
# without this, the resulting binary is unstripped.
runHook postInstall
postInstall = ''
install -D udpt.toml $out/usr/lib/udpt/udpt.toml
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
install -D udpt.toml $out/usr/lib/udpt/udpt.toml
install -D udpt.toml $out/lib/udpt/udpt.toml

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation and removed 10.rebuild-linux: 1 labels Jul 21, 2020
@makefu
Copy link
Contributor Author

makefu commented Jul 21, 2020

i went for $out/share/udpt/udpt.toml

flokli
flokli requested changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

I get hash mismatches in the fixed-output derivation both on ofborg and locally.

@makefu
Copy link
Contributor Author

makefu commented Jul 22, 2020 •
edited
Loading

@flokli okay this is weird, i get a different vendor checksum on the current nixpkgs master compared to nixos-20.03 ... were there some breaking changes on master regarding cargo checksum generation? i tested with e2bb73c (20.03) as well as 5507932 (master)

@flokli
Copy link
Contributor

flokli commented Jul 23, 2020

#79975 probably.

makefu added 2 commits July 23, 2020 17:29
@makefu
udpt: 2017-09-27 -> 3.1.1
282c558 
udpt did a complete rewrite from cpp to rust.
This Commit uses rust with cargo to build the package.
The Configuration format changed form .conf to .toml and
the binary changed from udpt to udpt-rs
@makefu
release-notes/rl-2009: add remark about udpt complete rewrite
ecdc10d
@makefu
Copy link
Contributor Author

makefu commented Jul 23, 2020

@flokli thanks for investigating! i've fixed the cargo hash.
Cheers!

@flokli flokli merged commit d5aa8ff into NixOS:master Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli flokli requested changes

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@makefu @flokli