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

mscgen: fix build #85580

Merged
merged 2 commits into from Apr 20, 2020
Merged

mscgen: fix build #85580

merged 2 commits into from Apr 20, 2020

Conversation

siriobalmelli
Copy link
Contributor

Motivation for this change

Found nix-env -iA "nixpkgs.mscgen" build was broken because of missing pkg-config dependency. Added it.

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 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
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested review from costrouc and peti April 19, 2020 20:32
calbrecht
calbrecht suggested changes Apr 19, 2020
View reviewed changes
Copy link
Member

@calbrecht calbrecht left a comment

Choose a reason for hiding this comment

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

Why are there changes in pkgs howdoi? Seems like this should go into a different PR.

@siriobalmelli
mscgen: add pkg-config build-time dependency
410fc69 
Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
@siriobalmelli
Copy link
Contributor Author

Why are there changes in pkgs howdoi? Seems like this should go into a different PR.

My apologies. They were in fact part of #85579 and I screwed up my branching.

Removed these entirely.

@ofborg ofborg bot removed the 6.topic: python label Apr 19, 2020
calbrecht
calbrecht suggested changes Apr 20, 2020
View reviewed changes
Copy link
Member

@calbrecht calbrecht left a comment •
edited

Choose a reason for hiding this comment

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

bison, pkg-config and flex should go into nativeBuildInputs instead.

@calbrecht
Copy link
Member

Then there is this notice when the tests beeing run

Warning: Optional UTF-8 byte-order-mark detected at start of input, but mscgen
         was not configured to use FreeType for text rendering.  Rendering of
         UTF-8 characters in PNG output may be incorrect.

Since you added freetype you might want to add

configureFlags = [ "--with-freetype" ]

and fix the Error: gdoTextWidth: Problem doing text layout (GDFONTPATH=)
or simply leave freetype out of dependencies.

@calbrecht
Copy link
Member

calbrecht commented Apr 20, 2020 •
edited

And the title of this PR should read like "mscgen: <your description>".

@calbrecht
Copy link
Member

And last, i guess. Consider adding outputs = [ "out" "man" ];. This way the man pages are linked correctly.

@siriobalmelli
mscgen: clean up build
3a7dca9 
1. Remove freetype dependency:
    - was not being used by build
    - trying to enable freetype gives nontrivial error
      'gdoTextWidth: Problem doing text layout (GDFONTPATH=)'

2. Correctly link manpages by specifying 'outputs'.

3. Separate nativeBuildInputs from buildInputs

Signed-off-by: Sirio Balmelli <sirio@b-ad.ch>
@siriobalmelli
Copy link
Contributor Author

And last, i guess. Consider adding outputs = [ "out" "man" ];. This way the man pages are linked correctly.

Thank you; added in 3a7dca9

@siriobalmelli siriobalmelli changed the title Fix/mscgen mscgen: fix build Apr 20, 2020
@siriobalmelli
Copy link
Contributor Author

bison, pkg-config and flex should go into nativeBuildInputs instead.

Thank you, fixed in 3a7dca9

@siriobalmelli
Copy link
Contributor Author

Then there is this notice when the tests beeing run

Warning: Optional UTF-8 byte-order-mark detected at start of input, but mscgen
         was not configured to use FreeType for text rendering.  Rendering of
         UTF-8 characters in PNG output may be incorrect.

Since you added freetype you might want to add

configureFlags = [ "--with-freetype" ]

and fix the Error: gdoTextWidth: Problem doing text layout (GDFONTPATH=)
or simply leave freetype out of dependencies.

Thank you.
I gave this a shot but no quick win in sight, so removed freetype dependency entirely in 3a7dca9

calbrecht
calbrecht approved these changes Apr 20, 2020
View reviewed changes
Copy link
Member

@calbrecht calbrecht left a comment

Choose a reason for hiding this comment

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

Good, thank you 👍

@Mic92
Copy link
Member

Mic92 commented Apr 20, 2020

Result of nixpkgs-review pr 85580 1

2 packages built:
- asciidoc-full-with-plugins
- mscgen

@Mic92 Mic92 merged commit 59242df into NixOS:master Apr 20, 2020
@siriobalmelli siriobalmelli deleted the fix/mscgen branch April 21, 2020 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 approved these changes

@calbrecht calbrecht calbrecht approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@costrouc costrouc Awaiting requested review from costrouc

@peti peti Awaiting requested review from peti

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@siriobalmelli @calbrecht @Mic92