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

texmaker: use wrapQtAppsHook #71675

Merged
merged 2 commits into from Oct 22, 2019
Merged

texmaker: use wrapQtAppsHook #71675

merged 2 commits into from Oct 22, 2019

Conversation

cruegge
Copy link
Contributor

@cruegge cruegge commented Oct 22, 2019

Motivation for this change

Texmaker on NixOS 19.09 is currently broken due to missing wrapQtAppsHook. I successfully tested the patch on the nixpkgs-channels/nixos-19.09 branch (at 80b42e6), but it fails to compile on master, presumably for unrelated reasons. I still rebased it according to the guidelines.

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 @markuskowa @Chaddai

@ofborg ofborg bot requested review from markuskowa and Chaddai October 22, 2019 14:43
@markuskowa
Copy link
Member

markuskowa commented Oct 22, 2019 •
edited

@GrahamcOfBorg build texmaker

@markuskowa markuskowa added the 9.needs: port to stable A PR needs a backport to the stable release. label Oct 22, 2019
@cruegge
Copy link
Contributor Author

cruegge commented Oct 22, 2019

Build failures are probably related to #71463. Should I wait for that to get resolved and rebase again?

@markuskowa
Copy link
Member

markuskowa commented Oct 22, 2019 •
edited

I am not sure this is related since this is the error: <command-line>: error: missing terminating " character but I can confirm that this PR fixes the build on 19.09.

The problem was introduced with merging staging-next into master. Let's merge this one first and then wait for the Qt problems to straighten out on master.

@ofborg ofborg bot requested a review from markuskowa October 22, 2019 15:21
@cruegge
Copy link
Contributor Author

cruegge commented Oct 22, 2019 •
edited

If I change the texmaker derivation along the lines of the patch mentioned in the other issue (83709b0), i.e. replace the preConfigure stuff by qmakeFlags and placeholder, it builds on master, and the result does contain the desktop file, icon and metainfo. I suppose there is some broken shell expansion / quoting to blame here.

I could add another commit that fixes this for texmaker (avoiding shell quoting is a good idea anyway), or open a new PR.

@markuskowa
Copy link
Member

A second commit that fixes it would be great.

@markuskowa markuskowa merged commit a51a718 into NixOS:master Oct 22, 2019
@markuskowa
Copy link
Member

Thanks for the fix!

@markuskowa
Copy link
Member

cherry picked for 19.09 in 56c4063

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Oct 22, 2019
@markuskowa @dtzWill
Merge pull request NixOS#71675 from cruegge/texmaker-wrapqtappshook
3bd7106 
texmaker: use wrapQtAppsHook
(cherry picked from commit a51a718)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chaddai Chaddai Awaiting requested review from Chaddai

@markuskowa markuskowa Awaiting requested review from markuskowa

Assignees
No one assigned
Labels
9.needs: port to stable A PR needs a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cruegge @markuskowa