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

envoy: 1.3.0 -> 1.11.1 #69279

Closed
wants to merge 1 commit into from
Closed

envoy: 1.3.0 -> 1.11.1 #69279

wants to merge 1 commit into from

Conversation

uri-canva
Copy link
Contributor

@uri-canva uri-canva commented Sep 23, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @cstrahan @abbradar @mboes

@@ -76,6 +76,11 @@ in stdenv.mkDerivation (fBuildAttrs // {
# For example, in Tensorflow-gpu build:
# platforms -> NIX_BUILD_TOP/tmp/install/35282f5123611afa742331368e9ae529/_embedded_binaries/platforms
find $bazelOut/external -maxdepth 1 -type l | while read symlink; do
# There is a top level symlink that points to the source directory, we need to keep that
target="$(readlink "$symlink")"
if [[ "$target" == "$NIX_BUILD_TOP/source" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Not all projects have source directory. Interesting that it's needed, maybe we need to filter /tmp symlinks instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it appears if they have external repositories that refer back to the root repository via @//path:name or @envoy//path:name.

Is the symlink removal here for symlinks to NIX_BUILD_TOP/tmp or to /tmp? Because if it's /tmp then we can delete all symlinks that point to outside of NIX_BUILD_TOP. But even if it points to NIX_BUILD_TOP/tmp why is that a problem? As long as it points inside NIX_BUILD_TOP rewriting the symlink the build part of the derivation should make it work?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that there is a hash in the path that changes from run to run (see an example above). Maybe we should drop everything to $NIX_BUILD_TOP/tmp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes I see. I would have expected --output_user_root to not add the hash. I guess because we control which directory that is we can then drop just the symlinks that point to that directory. Maybe we can rename it while we're at it, tmp didn't make it obvious that it was the output user root.

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.

looks like a few regressions, 6 of these fail on master (tensorflow-gpu has incorrect hash), but 19 fail on this PR

19 package failed to build:
libtensorflow python27Packages.baselines python27Packages.dm-sonnet python27Packages.edward python27Packages.graph_nets python27Packages.tensorflow python27Packages.tensorflow-probability python27Packages.tensorflowWithCuda python27Packages.tflearn python37Packages.baselines python37Packages.dm-sonnet python37Packages.edward python37Packages.graph_nets python37Packages.optuna python37Packages.rl-coach python37Packages.tensorflow python37Packages.tensorflow-probability python37Packages.tensorflowWithCuda python37Packages.tflearn

2 package were build:
bazel-watcher envoy

@flokli flokli mentioned this pull request Oct 14, 2019
9 tasks
@flokli
Copy link
Contributor

flokli commented Oct 14, 2019

@uri-canva the old and broken envoy and bazel_0_4 were removed in #71131.

Can you rebase this on top of latest master?

@uri-canva
Copy link
Contributor Author

Will do thanks!

@uri-canva
Copy link
Contributor Author

Still need to update the hashes of the derivations that use buildBazelPackage.

sed -e '/^FILE:@com_github_gperftools_gperftools/autom4te.cache.*/d' -i $bazelOut/external/\@*.marker
'';

sha256 = "1sq8qdbg8f94dbyc607xpq2yrhw61j3yxq6kggywp6fbbg89fh55";
Copy link
Contributor

@cpcloud cpcloud Nov 11, 2019

Choose a reason for hiding this comment

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

I'm trying to build this locally to verify that it works, and I'm getting a hash mismatch:

$ nix-build -f . -A envoy
...
INFO: All external dependencies fetched successfully.
Loading: 379 packages loaded
Loading: 379 packages loaded
Loading: 379 packages loaded
@nix { "action": "setPhase", "phase": "installPhase" }
installing
hash mismatch in fixed-output derivation '/nix/store/zr2djgq4yra1gyjvgdrmz4qjmfkl3x2m-envoy-1.11.1-deps':
  wanted: sha256:1sq8qdbg8f94dbyc607xpq2yrhw61j3yxq6kggywp6fbbg89fh55
  got:    sha256:0xlgw90m8sk47c6mr561hh7gbczsxlc8cqc2yr8xkv6vgdzydmz5

This was after rebasing onto upstream master, before which I saw a different hash for got:. Is it possible that not enough is being cleaned up here or somehow the rule isn't running?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible there's other files that need to be cleaned in there.

@uri-canva
Copy link
Contributor Author

I'm not using envoy and it was removed on master so I'll drop this. I was only updating it to clean up bazel-0.4.

@uri-canva uri-canva closed this Nov 12, 2019
@uri-canva uri-canva deleted the envoy branch November 12, 2019 00:09
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

5 participants