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

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

Merged
merged 2 commits into from Sep 3, 2020
Merged

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
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).

@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 sort platform than than they will be run 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)
jonringer pushed a commit 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 #95910.

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

Successfully merging this pull request may close these issues.

None yet

9 participants