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.

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

xxdiff: drop the old qt4 version in favour of qt5 #67498

Merged
merged 1 commit into from Sep 9, 2019

Conversation

@peterhoeg
Copy link
Member

commented Aug 26, 2019

Motivation for this change

We have been carrying the qt5 version for a long time (which works fine), so let's just drop the old qt4 variant. Also use the proper qt5 mkDerivation.

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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @pSub @7c6f434c

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@GrahamcOfBorg build xxdiff xxdiff-tip

@peterhoeg peterhoeg requested a review from pSub Aug 26, 2019
@ofborg ofborg bot requested a review from 7c6f434c Aug 26, 2019
@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@GrahamcOfBorg build xxdiff

@peterhoeg peterhoeg force-pushed the peterhoeg:f/xxdiff branch from dfba654 to 123f72c Aug 27, 2019
@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2019

@GrahamcOfBorg build xxdiff

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

What's going on with the source?

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

What's going on with the source?

Do you mind trying to do nix-prefetch-url on your side? When I do it here, I get the same sum as as in the PR.

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Wait, nix-prefetch-url on what? The new expression uses fetchFromBitbucket, which unpacks.

Local build produces the same hash mismatch error as ofBorg.

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

Wait, nix-prefetch-url on what? The new expression uses fetchFromBitbucket, which unpacks.
Local build produces the same hash mismatch error as ofBorg.

That's so strange - it works perfectly fine here.

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

And if you GC the output of xxdiff.src, does everything still build afterwards?

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

And if you GC the output

You're a star! That did it.

@peterhoeg peterhoeg force-pushed the peterhoeg:f/xxdiff branch from 123f72c to 10ef8f5 Aug 30, 2019
@7c6f434c

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Why build-time sourceRoot is any different from doing cd src in preConfigure?

@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

Why build-time sourceRoot is any different from doing cd src in preConfigure?

Because if we set sourceRoot, only the src directory gets copied over to the build directory so we no longer have access to the README that needs copying in at install time. Additionally, we would have to patch the make files to not place the output in ../bin.

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@peterhoeg peterhoeg force-pushed the peterhoeg:f/xxdiff branch from 10ef8f5 to 34a09c3 Sep 9, 2019
@peterhoeg peterhoeg force-pushed the peterhoeg:f/xxdiff branch from 34a09c3 to c782160 Sep 9, 2019
@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

This is not what is defined in setup.sh

Which setup.sh?

and not what happens in practice if I set sourceRoot to be source/src

The README file is outside the source directory after it has been unpacked/copied over, but we can still get to it via ${src}/README.

Additionally, we would have to patch the make files to not place the output in ../bin.
Ah right, writing outside sourceRoot is a problem as it is still read-only. But I guess this is what the comment should say, because right now it looks strange.

I've cleaned it up. Let me know what you think.

@7c6f434c

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

This is not what is defined in setup.sh

Which setup.sh?

pkgs/stdenv/generic/setup.sh

and not what happens in practice if I set sourceRoot to be source/src

The README file is outside the source directory after it has been unpacked/copied over, but we can still get to it via ${src}/README.

Actually, ../README also works, but whatever you consider cleaner.

Unpack phase copies entire ${src}, but then only ./${sourceRoot} is made writeable.

I've cleaned it up. Let me know what you think.

Nice, thanks.

@peterhoeg peterhoeg merged commit 65fb1a0 into NixOS:master Sep 9, 2019
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@peterhoeg peterhoeg deleted the peterhoeg:f/xxdiff branch Sep 9, 2019
@peterhoeg

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

Unpack phase copies entire ${src}, but then only ./${sourceRoot} is made writeable.

I must have missed something then. I'm sure I tried the ../README.md bit as well. Oh well. It works, everybody is happy! Thanks man!

@peterhoeg peterhoeg restored the peterhoeg:f/xxdiff branch Sep 9, 2019
@peterhoeg peterhoeg deleted the peterhoeg:f/xxdiff branch Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.