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

WIP KDE5: fix breakage due to set -u changes #73756

Merged
merged 2 commits into from Nov 24, 2019
Merged

WIP KDE5: fix breakage due to set -u changes #73756

merged 2 commits into from Nov 24, 2019

Conversation

@FRidh
Copy link
Member

FRidh commented Nov 19, 2019

Motivation for this change
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 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 @

@FRidh FRidh requested review from Mic92, ttuegel and Ericson2314 Nov 19, 2019
This was referenced Nov 19, 2019
@FRidh

This comment has been minimized.

Copy link
Member Author

FRidh commented Nov 19, 2019

I don't know why konsole was broken. It seems odd to me. Maybe the fix in the hook is not correct. Checking other KDE components now as well.

@FRidh

This comment has been minimized.

Copy link
Member Author

FRidh commented Nov 19, 2019

Adding dependencies like I did is I think the wrong way because most KDE applications are now broken. That has to be due to propagation or the hook.

Copy link
Member

Ericson2314 left a comment

Looks good. FWIW I've been trying to do ${foo-} rather than ${foo:-} as they are the same (only matters when something is right of the -), and I decided the latter is more complex for the same job.

@FRidh FRidh force-pushed the FRidh:kde5 branch from 5db86b2 to 88dc84c Nov 20, 2019
@FRidh

This comment has been minimized.

Copy link
Member Author

FRidh commented Nov 20, 2019

I've pushed the hooks to staging-next.

@ofborg ofborg bot removed the 6.topic: python label Nov 20, 2019
@FRidh FRidh changed the base branch from staging-next to master Nov 20, 2019
@FRidh FRidh changed the title KDE5: fix breakage due to set -u changes WIP KDE5: fix breakage due to set -u changes Nov 20, 2019
@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Nov 21, 2019

Are these remaining changes set -u related?

@FRidh

This comment has been minimized.

Copy link
Member Author

FRidh commented Nov 21, 2019

Well, nothing else has changed anywhere close to KDE packaging as far as I can tell.

@FRidh

This comment has been minimized.

Copy link
Member Author

FRidh commented Nov 21, 2019

Bisecting with this large staging-next change is not going to be fun

@FRidh FRidh mentioned this pull request Nov 22, 2019
@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Nov 23, 2019

@FRidh Or simplier put, what is the symptom this fixes, and how do I reproduce this on master today?

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 23, 2019

@Ericson2314 nix-build -A kdeApplications.konsole:

https://hydra.nixos.org/build/107199242/nixlog/1

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 23, 2019

git bisect points to 13bab29, perhaps the hookName should be properly escaped. Though I do not see how it is not a syntax error.

@FRidh

This comment has been minimized.

Copy link
Member Author

FRidh commented Nov 23, 2019

Yes, it's missing the ''. Did a successful build with that fixed.

@jtojnar

This comment has been minimized.

Copy link
Contributor

jtojnar commented Nov 23, 2019

Are you pushing the fix?

@FRidh

This comment has been minimized.

Copy link
Member Author

FRidh commented Nov 23, 2019

@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Nov 23, 2019

Should use [[ too as you will get a problem of too few args with empty string.

@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Nov 23, 2019

Also consider plain ${foo-} to rule out the URL possibility.

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Nov 24, 2019

Having problems with kwin like

building '/nix/store/svr5q5q3i7ja9wv0d23brbs2bdcp092c-kwin-5.16.5.drv'...
/nix/store/mq2hcss5fc5g3127z22abpy0a3lk9zk0-breeze-qt5-5.16.5-dev/nix-support/setup-hook: line 5: propagatedUserEnvPkgs: unbound variable
builder for '/nix/store/svr5q5q3i7ja9wv0d23brbs2bdcp092c-kwin-5.16.5.drv' failed with exit code 1
error: build of '/nix/store/svr5q5q3i7ja9wv0d23brbs2bdcp092c-kwin-5.16.5.drv' failed

currently blocking the channel.

@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Nov 24, 2019

Well those responses were for the linked commits. This PR is fine; I still don't know why it became necessary, but the deep dives we've done into other issues didn't turn up fundimental problems, so I'll just let my lack of knowledge here slide.

@Ericson2314 Ericson2314 merged commit 10a0a23 into NixOS:master Nov 24, 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
@FRidh

This comment has been minimized.

Copy link
Member Author

FRidh commented Nov 24, 2019

this should not have gone in because these changes were not needed

@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Nov 24, 2019

Oh. I'm sorry, I thought they were. Should have remembered the WIP.

@FRidh FRidh deleted the FRidh:kde5 branch Nov 25, 2019
@FRidh

This comment has been minimized.

Copy link
Member Author

FRidh commented Nov 25, 2019

latest fix 646b279

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.