Skip to content

Comments

texinfo: 6.5 -> 6.7; fix cross compiling#95910

Merged
Mic92 merged 2 commits intoNixOS:stagingfrom
kampka:texinfo
Sep 3, 2020
Merged

texinfo: 6.5 -> 6.7; fix cross compiling#95910
Mic92 merged 2 commits intoNixOS:stagingfrom
kampka:texinfo

Conversation

@kampka
Copy link
Contributor

@kampka kampka commented Aug 21, 2020

Motivation for this change

The current derivation does not correctly cross-build when building in non-interactive mode.
Looking at the derivation, I don't see a reason for this distinction at all, hence this PR removes 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.

@kampka
Copy link
Contributor Author

kampka commented Aug 21, 2020

@GrahamcOfBorg build texinfo

@kampka kampka requested a review from thefloweringash August 21, 2020 15:30
@kampka
Copy link
Contributor Author

kampka commented Aug 21, 2020

@thefloweringash according to git blame, you added the interactive cross-build check. Maybe you can shed some light on this?

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

since this change causes over 500 rebuilds, do you mind targeting the staging branch

git rebase --onto=staging HEAD~1
git push .. ... --force

then change the base branch in the github PR from master -> staging

See https://nixos.org/nixpkgs/manual/#submitting-changes-staging-branch for more details on staging branch

$ nix-env -f /home/jon/.cache/nixpkgs-review/pr-95910/nixpkgs -qaP --xml --out-path --show-trace --meta
28975 packages updated:

@kampka
Copy link
Contributor Author

kampka commented Aug 22, 2020

@GrahamcOfBorg build texinfo

@kampka
Copy link
Contributor Author

kampka commented Aug 22, 2020

@jonringer Done.
Due to the massive rebuilds, I cannot claim that this change is well tested already, I can just claim that it builds on x86 and cross-aarch64 and that a couple of choice dependencies work as well (on master).

@kampka kampka requested a review from jonringer August 22, 2020 12:32
@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 22, 2020
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Aug 22, 2020
@ofborg ofborg bot requested review from edolstra, lovek323, np, oxij and vrthra and removed request for jonringer and zowoq August 22, 2020 14:51
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. and removed 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. labels Aug 22, 2020
@oxij
Copy link
Member

oxij commented Aug 22, 2020 via email

@Ericson2314
Copy link
Member

if it is just used for bootstrapping, maybe the cross + non-interactive case won't occur very often in practice.

That cross patch looks very innocuous. Maybe we should try to always apply it? This is a mass rebuild already.

@kampka
Copy link
Contributor Author

kampka commented Aug 22, 2020

@oxij This change does not remove interactive mode, it just removes the flag that determines that cross-build patches are only applied on interactive mode. Now, they are applied on both, my question is, why where they separated in the first place in regards to cross building?

@bjornfor bjornfor added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Aug 22, 2020
@thefloweringash
Copy link
Member

@thefloweringash according to git blame, you added the interactive cross-build check. Maybe you can shed some light on this?

It was a while ago, but as far as I can tell only the interactive case was broken and it avoided a mass rebuild according to the tags on #76598. I don’t know what changed to make this now required for the non-interactive case, but it also fails for me on master.

Testing as of 6dd60c6
$ git checkout 6dd60c6cac8abc95f13f8bdd068d2c18865ca8b7
[...]
HEAD is now at 6dd60c6cac8 texinfoInteractive: fix cross build

$ nix-build -A 'pkgsCross.aarch64-multiplatform.texinfo' -A 'pkgsCross.aarch64-multiplatform.texinfoInteractive'
/nix/store/4pzd5442f0l43xzhwjfh7riir4pk872l-texinfo-6.5-aarch64-unknown-linux-gnu
/nix/store/d68f5rxc8582l2vz6b9c8rwkfjmybv9f-texinfo-interactive-6.5-aarch64-unknown-linux-gnu

@kampka
Copy link
Contributor Author

kampka commented Aug 27, 2020

Is there any way to get this patch improved to move it along? I know it's a bit last minute, but I'd really hate to miss the 20.09 release window with this ...

@Mic92 Mic92 merged commit 7a0a9a5 into NixOS:staging Sep 3, 2020
@FRidh
Copy link
Member

FRidh commented Sep 5, 2020

This broke darwin stdenv and is blocking staging-next (and branch-off thereby) https://nix-cache.s3.amazonaws.com/log/lnkwicmp12bcy9fhdillhkwh2xm9px8b-texinfo-6.7.drv

@FRidh
Copy link
Member

FRidh commented Sep 5, 2020

Proposed revert #97218.

@FRidh FRidh mentioned this pull request Sep 5, 2020
10 tasks
@kampka
Copy link
Contributor Author

kampka commented Sep 5, 2020

Proposed revert #97218.

This PR contains two commits, the cross-compile fixes and the version bump, the later of which seems to introduce this regression.
I think it should be fairly easy to patch this forward, but if you want to revert, would it make sense to just revert the version bump (I'm mostly interested in the cross-compile fixes anyways).
Since I cannot test against Darwin, I'm afraid I cannot offer much active help in form of testing.

@FRidh
Copy link
Member

FRidh commented Sep 5, 2020

A fix is preferred, and otherwise its a revert. I was under the impression it was the interactive change, but did not give it much thought.

I can't test it myself either, like most of us...

@vcunat
Copy link
Member

vcunat commented Sep 5, 2020

Or postpone this whole PR after 20.09? I can't see why it needs to be in there as a last-minute change. (I can't test Darwin either.)

vcunat added a commit that referenced this pull request Sep 6, 2020
It's basically a partial revert of PR #95910.
I chose a temporar-ish solution, maximizing likelihood of fixing
this while minimizing rebuilds.  (20.09 process is being blocked)
orivej-nixos pushed a commit that referenced this pull request Sep 18, 2020
makeinfo seems right to fail when input encoding is not declared and is not UTF-8.

texinfo was updated in #95910.
risicle pushed a commit to risicle/nixpkgs that referenced this pull request Sep 20, 2020
makeinfo seems right to fail when input encoding is not declared and is not UTF-8.

texinfo was updated in NixOS#95910.

(cherry picked from commit 19f7f15)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: cross-compilation Building packages on a different platform than they will be used on 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants