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

wordgrinder: Fix build #60122

Closed
wants to merge 1 commit into from
Closed

wordgrinder: Fix build #60122

wants to merge 1 commit into from

Conversation

matthiasbeyer
Copy link
Contributor

This fixes the build of the wordgrinder package by disabling the build
of "xwordgrinder", the non-curses frontend of wordgrinder.

For re-enabling the build, this patch shall be reverted and then go on
from there.

I opted for disabling the x frontend of this program because the whole
purpose of this is to use a curses writing program (at least for me).
It seems that hacking around the new build system in nixpkgs is a
non-trivial task and I wanted to get this package out of the door as
fast as possible because I already delayed the fix by a few days (or is
it weeks already), so this is the simple fix.

If someone wants to have the X frontend again, I can invest more time at
some point.

Fixes: 6b2bd33 ("wordgrinder: Fix sha256 hash")
Fixes: 19a3abc ("wordgrinder: 0.7.1 -> 0.7.2")
CC: @aszlig
CC: @devhell

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@aszlig
Copy link
Member

aszlig commented Apr 23, 2019 •
edited

The XFT_PACKAGE=--libs=\{-lX11 -lXft\} construct was never really valid in the first place, because it was subject to shell splitting even prior to 0.7.2.

What has changed in this regard is not the way builds are handled in nixpkgs1, but it seems that 0.7.2 now is checking whether those flags are actually correct. Using XFT_PACKAGE=--libs={-lX11 -lXft} (btw. those backslashes used before are a no-op within "-style strings) the passed argument to make is actually XFT_PACKAGE=--libs={-lX11 and the build worked anyway because later in build.lua, the linker arguments are queried via pkg-config regardless of what you pass to make.

So using makeFlagsArray gets rid of the shell splitting.

1 It might however change one day if we leverage __structuredAttrs in stdenv so that we no longer have the shell splitting issue.

@matthiasbeyer
Copy link
Contributor Author

Ready for squashing here.

aszlig
aszlig requested changes Apr 23, 2019
View reviewed changes
pkgs/applications/office/wordgrinder/default.nix Outdated
@@ -18,6 +18,10 @@ stdenv.mkDerivation rec {
"LUA_LIB=${lua52Packages.lua}/lib/liblua.so"
] ++ stdenv.lib.optional stdenv.isLinux "XFT_PACKAGE=--libs=\{-lX11 -lXft\}";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
] ++ stdenv.lib.optional stdenv.isLinux "XFT_PACKAGE=--libs=\{-lX11 -lXft\}";
];

Copy link
Member

Choose a reason for hiding this comment

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

We already have this in preRebuild, so no need to duplicate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hang on...

@matthiasbeyer
Copy link
Contributor Author

Ready here, if you (@aszlig) don't mind the commit message 😄

@matthiasbeyer
wordgrinder: Fix build
45841f5 
First I tried to fix the build like this:

> This fixes the build of the wordgrinder package by disabling the build
> of "xwordgrinder", the non-curses frontend of wordgrinder.
>
> For re-enabling the build, this patch shall be reverted and then go on
> from there.
>
> I opted for disabling the x frontend of this program because the whole
> purpose of this is to use a curses writing program (at least for me).
> It seems that hacking around the new build system in nixpkgs is a
> non-trivial task and I wanted to get this package out of the door as
> fast as possible because I already delayed the fix by a few days (or is
> it weeks already), so this is the simple fix.
>
> If someone wants to have the X frontend again, I can invest more time at
> some point.

But then the almighty aszlig came around and told me how to do it right. So here
we go, with a patch from me that is acutally from aszlig (see NixOS#60122).

This fixes the build.

Suggested-by: @aszlig
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Fixes: 6b2bd33 ("wordgrinder: Fix sha256 hash")
Fixes: 19a3abc ("wordgrinder: 0.7.1 -> 0.7.2")
CC: @aszlig
CC: @devhell
@matthiasbeyer
Copy link
Contributor Author

This can be merged, right?

aszlig pushed a commit that referenced this pull request Apr 26, 2019
@matthiasbeyer @aszlig
wordgrinder: Fix build
31bee0c 
First I tried to fix the build like this:

> This fixes the build of the wordgrinder package by disabling the build
> of "xwordgrinder", the non-curses frontend of wordgrinder.
>
> For re-enabling the build, this patch shall be reverted and then go on
> from there.
>
> I opted for disabling the x frontend of this program because the whole
> purpose of this is to use a curses writing program (at least for me).
> It seems that hacking around the new build system in nixpkgs is a
> non-trivial task and I wanted to get this package out of the door as
> fast as possible because I already delayed the fix by a few days (or is
> it weeks already), so this is the simple fix.
>
> If someone wants to have the X frontend again, I can invest more time at
> some point.

But then the almighty aszlig came around and told me how to do it right. So here
we go, with a patch from me that is acutally from aszlig (see #60122).

This fixes the build.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: aszlig <aszlig@nix.build>
Fixes: 6b2bd33 ("wordgrinder: Fix sha256 hash")
Fixes: 19a3abc ("wordgrinder: 0.7.1 -> 0.7.2")
Merges: #60122
Cc: @devhell
@aszlig
Copy link
Member

aszlig commented Apr 26, 2019

Merged in 31bee0c.

@aszlig aszlig closed this Apr 26, 2019
@matthiasbeyer matthiasbeyer deleted the fix-wordgrinder branch April 26, 2019 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aszlig aszlig aszlig approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@matthiasbeyer @aszlig