flatbuffers: 2.0.0 -> 2.0.6

[smancill]

Description of changes

Redo of #170185, fixing the tests.

• Looks like the tests need access to files in the <srcdir>/tests directory, so this path needs to be patched now. (Apparently upstream uses cmake ., and something broke between the versions for out of source builds.)

• Python is needed to generate tests, according to CMake output.

• Disable generation of universal binaries on Darwin.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[risicle]

nixpkgs-review reveals no new failures on macos 10.15.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/571

[samuela]

This broke tensorflow and its entire ecosystem of packages (#185273). @smancill can you please fix these build failures or revert this PR?

[SuperSandro2000]

Before suggesting a revert please always check upstream for fixes. Those commits seem to fix the compatibility with flatbuffers.

tensorflow/tensorflow@bf0901b
tensorflow/tensorflow@625a404

But I assume this was broken since the flatbuffers 2.0.0 update and the patches are quite big. Tensorflow 2.10.0 seems to be released soonish. If that takes to long I would suggest to bring back flatbuffers_1 instead of doing a revert.

Collecting this information took me no more than 5 minutes and something reasonable to expect from a maintainer.

[samuela]

@SuperSandro2000 As discussed in #185079 and elsewhere the policy is for PRs targeting master to go from "green to green". That means no downstream failures. Therefore, errant PRs should be reverted if they are found to cause failures.

The burden of maintenance should not be unfairly thrust upon downstream maintainers (@jyp and @abbradar in this case), esp. when committing straight to master.

Collecting this information took me no more than 5 minutes and something reasonable to expect from a maintainer.

Agreed, and that's why it is reasonable to expect it to be done by the authors/mergers of this PR prior to merging.

[trofi]

Yeah, I should have ran nix-review. Is tensorflow a reasonable test depend for flatbuffers to ease catching it via ofborg next time? Or is it too heavyweight?

[samuela]

Yeah, I should have ran nix-review. Is tensorflow a reasonable test depend for flatbuffers to ease catching it via ofborg next time? Or is it too heavyweight?

Tensorflow is fairly heavyweight, but I'm not sure what the ofborg build limits are exactly... IIRC tensorflow should be ~6hrs on a 4 vCPU machine

[smancill]

Well, it's bad that tensorflow is broken, but that has nothing to do with the contributor/maintainer that happens to update flatbuffers. It's completely unreasonable to ask PR authors or reviewers to build hundreds of packages on their personal machine just to make sure downstream derivations are not broken, and even more unreasonable to ask them to do maintenance of those derivations if something is broken.

This didn't break any policy, neither. It can target master given the number of dependencies, as it is documented; and there is no requirement to always run nipkgs-review before merging, it's just a suggestion to help reviews when it is reasonable to run it on personal machines.

The master branch is unstable, with the best effort to minimize breakage, but temporary breakage can and will occur sometimes.

And it's completely ridiculous that a patch release of flatbuffers can break tensorflow. That's on the Google devs, not nixpkgs contributors.

[SuperSandro2000]

elsewhere the policy is for PRs targeting master to go from "green to green". That means no downstream failures. Therefore, errant PRs should be reverted if they are found to cause failures.

Yes, you are right but its merged now and it would be pointless and make the git history really awkward to revert it now and then revert the revert to introduce flatbuffers_1. Next time something to look out.

tensorflow should be ~6hrs on a 4 vCPU machine

That's really long and I can fully understand anyone that does not want to run such a long build.
Can we move the tests to passthru so that a normal build is faster and more people can run it? If it convenient then people are more likely to check it and fix it.

[samuela]

Well, it's bad that tensorflow is broken, but that has nothing to do with the contributor/maintainer that happens to update flatbuffers. It's completely unreasonable to ask PR authors or reviewers to build hundreds of packages on their personal machine just to make sure downstream derivations are not broken, and even more unreasonable to ask them to do maintenance of those derivations if something is broken.

For a package with as many reverse transitive dependencies as flatbuffers it is reckless to target master without running nixpkgs-review. If running nixpkgs-review is too onerous, then one should target staging instead.

[Mic92]

I fixed tensorflow for now by upgrading it to the latest release candidate, but the package still remains broken on darwin.