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

newsboat: 2.20.1 -> 2.21 #98471

Merged
merged 1 commit into from Oct 5, 2020
Merged

newsboat: 2.20.1 -> 2.21 #98471

merged 1 commit into from Oct 5, 2020

Conversation

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Sep 22, 2020

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 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.
@sikmir
Copy link
Member

@sikmir sikmir commented Sep 22, 2020

Broken on darwin:

  1. Add Foundation framework to buildInputs
  2. Another error:
error: could not find native static library `intl`, perhaps an -L flag is missing?

error: aborting due to previous error

error: could not compile `gettext-sys`.

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Sep 22, 2020

Maybe it'll work now.

@sikmir
Copy link
Member

@sikmir sikmir commented Sep 23, 2020

Maybe it'll work now.

No, adding intltool doesn't help.

@jonringer
Copy link
Contributor

@jonringer jonringer commented Oct 1, 2020

@doronbehar unless you're collaborating with others, please avoid pushing PRs to nixpkgs/nixos

it's common for me to "reset" an experiment by doing git checkkout pk<tab>, and now it defaults to this branch each time

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 1, 2020

@doronbehar unless you're collaborating with others, please avoid pushing PRs to nixpkgs/nixos

it's common for me to "reset" an experiment by doing git checkkout pk<tab>, and now it defaults to this branch each time

Yea I know! I noticed that only after the PR was open and had reviews. Sorry 😬 . I will write a pre-push hook to avoid it, and remember to delete the branch if/when this PR is merged / closed.

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 1, 2020

@sikmir I couldn't find evidence for upstream issues with 2.21 and Darwin :/. Are you using actively this newsboat? If yes, maybe we should get help from upstream. If not, I think I'll mark it as broken for Darwin, if we won't find solution. I could also use some help with 2 TODOs in the current expression regarding seemingly old / not currently needed Darwin workarounds. Current version of the PR is marking it as broken, with an added comment for a Darwin specific thing.

@sikmir
Copy link
Member

@sikmir sikmir commented Oct 1, 2020

@doronbehar Yes, I would like to use it on darwin as well and now only 2.19 works for me (1316637). But I also have no idea what is wrong with latest versions...

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 1, 2020

@sikmir could you try set in the derivation this?:

GETTEXT_SYSTEM = true;

I'm trying to follow this.

@sikmir
Copy link
Member

@sikmir sikmir commented Oct 2, 2020

@sikmir could you try set in the derivation this?:

GETTEXT_SYSTEM = true;

I'm trying to follow this.

The same result:

..
   Compiling gettext-rs v0.5.0
     Running `rustc --crate-name gettextrs /private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/newsboat-2.21-vendor.tar.gz/gettext-rs/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -Cembed-bitcode=no -C metadata=5a6fb6ac89878a0e -C extra-filename=-5a6fb6ac89878a0e --out-dir /private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/source/target/release/deps -C linker=/nix/store/sw7b2gcklaj3iy5ikv7ag163bpry3ivb-clang-wrapper-7.1.0/bin/cc -L dependency=/private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/source/target/release/deps --extern gettext_sys=/private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/source/target/release/deps/libgettext_sys-00750fbde98dd8d4.rmeta --extern locale_config=/private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/source/target/release/deps/liblocale_config-daaaa1299af9fb6f.rmeta --cap-lints allow -L native=/private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/source/target/release/build/gettext-sys-d162d66149a94350/out/lib`
error: could not find native static library `intl`, perhaps an -L flag is missing?

error: aborting due to previous error

error: could not compile `gettext-sys`.

Caused by:
  process didn't exit successfully: `rustc --crate-name gettext_sys /private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/newsboat-2.21-vendor.tar.gz/gettext-sys/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts --crate-type lib --emit=dep-info,metadata,link -C opt-level=3 -Cembed-bitcode=no --cfg 'feature="gettext-system"' -C metadata=00750fbde98dd8d4 -C extra-filename=-00750fbde98dd8d4 --out-dir /private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/source/target/release/deps -C linker=/nix/store/sw7b2gcklaj3iy5ikv7ag163bpry3ivb-clang-wrapper-7.1.0/bin/cc -L dependency=/private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/source/target/release/deps --cap-lints allow -L native=/private/var/folders/wf/qxrs82v946n91pb5s71n3dy80000gp/T/nix-build-newsboat-2.21.drv-0/source/target/release/build/gettext-sys-d162d66149a94350/out/lib -l framework=CoreFoundation -l dylib=iconv -l static=intl` (exit code: 1)
warning: build failed, waiting for other jobs to finish...
error: build failed
make: *** [Makefile:108: target/release/libnewsboat.a] Error 101
builder for '/nix/store/bibz7681a2kfi4nmcn4gb494q68xil8z-newsboat-2.21.drv' failed with exit code 2
error: build of '/nix/store/bibz7681a2kfi4nmcn4gb494q68xil8z-newsboat-2.21.drv' failed

@Minoru
Copy link
Contributor

@Minoru Minoru commented Oct 3, 2020

Hi! Newsboat maintainer here 👋 As of recently, I also co-maintain gettext-rs and gettext-sys crates, so I guess I am responsible for these build failures no matter how you look at it :)

GETTEXT_SYSTEM=true won't do anything on macOS, because unlike glibc, the macOS libc doesn't contain gettext functions. So on macOS, you have two options:

  1. let gettext-sys compile a copy of GNU gettext for you;
  2. install GNU gettext library and point at it with GETTEXT_DIR or GETTEXT_{LIB,INCLUDE,BIN}_DIR.

Right now, option 1 takes effect (and fails). Can you please try option 2?

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 3, 2020

Hi! Newsboat maintainer here wave As of recently, I also co-maintain gettext-rs and gettext-sys crates, so I guess I am responsible for these build failures no matter how you look at it :)

Wow you are one responsible upstream maintainer :) Thanks for joining in!

2\. install GNU gettext library and point at it with `GETTEXT_DIR` or `GETTEXT_{LIB,INCLUDE,BIN}_DIR`.

I updated the PR with something that perhaps will work. @sikmir if you could recheck please 🙏 .

@Minoru
Copy link
Contributor

@Minoru Minoru commented Oct 3, 2020

@doronbehar It looks like you forgot to git push the update? ;)

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 3, 2020

Yes I did (we had a branches issue as well here). Thanks for the quick catch!

@Minoru
Copy link
Contributor

@Minoru Minoru commented Oct 3, 2020

No problem! The changes look good to me. I have a similar configuration in Newsboat's CI, so I'm pretty confident this would work. (My CI also sets LDFLAGS and CXXFLAGS, I'm not sure if this would be needed for NixOS since you built previous Newsboat releases without these flags just fine.)

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 3, 2020

My CI also sets LDFLAGS and CXXFLAGS

Our "stdenv" sets them automatically according to what's in the build inputs :) (to put it simply).

Add missing deps for Darwin, and tell it where gettext is installed.
@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 3, 2020

Have set the flags for all platforms now, and removed the broken meta attribute.

@sikmir
Copy link
Member

@sikmir sikmir commented Oct 5, 2020

I updated the PR with something that perhaps will work. @sikmir if you could recheck please pray .

@doronbehar yeah, it finally works on darwin!

@Minoru thanks for your help!

@doronbehar doronbehar merged commit 5267988 into master Oct 5, 2020
19 of 20 checks passed
@doronbehar doronbehar deleted the pkg/newsboat branch Oct 5, 2020
@sikmir
Copy link
Member

@sikmir sikmir commented Oct 5, 2020

Looks like -Wno-error=format-security flag not needed anymore on darwin. Can be removed with next update.

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 5, 2020

Thanks again @sikmir, will remember to remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants