Skip to content

envoy 1.24.0#113451

Closed
chenrui333 wants to merge 1 commit intoHomebrew:masterfrom
chenrui333:bump-envoy-1.23.2
Closed

envoy 1.24.0#113451
chenrui333 wants to merge 1 commit intoHomebrew:masterfrom
chenrui333:bump-envoy-1.23.2

Conversation

@chenrui333
Copy link
Copy Markdown
Member

Created by brew bump


Created with brew bump-formula-pr.

@BrewTestBot BrewTestBot added the bump-formula-pr PR was created using `brew bump-formula-pr` label Oct 18, 2022
@dio
Copy link
Copy Markdown
Contributor

dio commented Oct 19, 2022

Thanks, @chenrui333!

This is interesting. The test caught by an error since: [-Werror,-Wrange-loop-analysis].

Details

external/v8/src/compiler/js-native-context-specialization.cc:2333:23: error: loop variable 'map' of type 'const v8::internal::compiler::MapRef' creates a copy from type 'const v8::internal::compiler::MapRef' [-Werror,-Wrange-loop-analysis]
for (const MapRef map : access_info.lookup_start_object_maps()) {
^
external/v8/src/compiler/js-native-context-specialization.cc:2333:10: note: use reference type 'const v8::internal::compiler::MapRef &' to prevent copying
for (const MapRef map : access_info.lookup_start_object_maps()) {
^~~~~~~~~~~~~~~~~~
&
1 error generated.

This is interesting since the previous version (1.23.1) depends on the same v8 version (10.4.132.18). Do we have a different compiler version now? (probably this one is clang 12?)

We have two options:

  1. Submit another patch to fix v8 (envoy patches v8)
  2. Create a user.bazelrc file and fill it with build:macos --copt='-Wno-error=range-loop-analysis' line. This can also be done by adding that line to .bazelrc.

Patch for 1.

diff --git a/bazel/v8.patch b/bazel/v8.patch
index 03c849156a..1b7a8f27b1 100644
--- a/bazel/v8.patch
+++ b/bazel/v8.patch
@@ -363,3 +363,16 @@ index 131ff9614e..6455f8757d 100644
      char filename[] = "/tmp/v8_tmp_file_for_testing_XXXXXX";
      fd = mkstemp(filename);
      if (fd != -1) CHECK_EQ(0, unlink(filename));
+diff --git a/src/compiler/js-native-context-specialization.cc b/src/compiler/js-native-context-specialization.cc
+index 729d8ac99f..c1b0f7e0ba 100644
+--- a/src/compiler/js-native-context-specialization.cc
++++ b/src/compiler/js-native-context-specialization.cc
+@@ -2330,7 +2330,7 @@ Node* JSNativeContextSpecialization::InlinePropertyGetterCall(
+ 
+   if (access_info.IsDictionaryProtoAccessorConstant()) {
+     // For fast mode holders we recorded dependencies in BuildPropertyLoad.
+-    for (const MapRef map : access_info.lookup_start_object_maps()) {
++    for (const MapRef& map : access_info.lookup_start_object_maps()) {
+       dependencies()->DependOnConstantInDictionaryPrototypeChain(
+           map, access_info.name(), constant, PropertyKind::kAccessor);
+     }

@dio
Copy link
Copy Markdown
Contributor

dio commented Oct 19, 2022

Ah, sorry, we re-enabled wasm support with the v8 engine (default) here: envoyproxy/envoy@78cccd3. Hence, this happens.

I vote for: creating user.bazelrc with the following content:

build:macos --copt='-Wno-error=range-loop-analysis'

Since it will be cleaner and when we do envoy --version it will print "Clean" instead of "Modified".

@dio
Copy link
Copy Markdown
Contributor

dio commented Oct 19, 2022

Also, 1.24.0 envoyproxy/envoy#23561 is happening.

@chenrui333
Copy link
Copy Markdown
Member Author

Also, 1.24.0 envoyproxy/envoy#23561 is happening.

yeah, gonna try that.

@chenrui333 chenrui333 mentioned this pull request Oct 21, 2022
@chenrui333 chenrui333 added the CI-requeued PR has been re-added to the queue label Oct 22, 2022
@chenrui333 chenrui333 changed the title envoy 1.23.2 envoy 1.24.0 Oct 24, 2022
Signed-off-by: Rui Chen <rui@chenrui.dev>
@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Oct 24, 2022
@dio
Copy link
Copy Markdown
Contributor

dio commented Oct 26, 2022

@chenrui333 I have successfully tested the following formula on Catalina, Monterey, and Ventura locally:

Feel free to modify this PR with it. Or please let me know if you want me to submit another PR.

Also, seems like we need to move the "versioned" envoy@1.18 one (to 1.23.2 (?)) after 1.24.0 is successfully built as "stable" since the only reason we have that was wasm support (1.24.0 has wasm support).

Thank you!

@dio
Copy link
Copy Markdown
Contributor

dio commented Oct 27, 2022

@chenrui333 sorry, do you want to modify this PR, or I can submit another one? Thank you!

@dio dio mentioned this pull request Oct 28, 2022
6 tasks
@chenrui333
Copy link
Copy Markdown
Member Author

@chenrui333 sorry, do you want to modify this PR, or I can submit another one? Thank you!

yeah, sounds good to me (always feel free to submit a new one :) )

@chenrui333 chenrui333 closed this Nov 1, 2022
@chenrui333 chenrui333 added the superseded PR was replaced by another PR label Nov 1, 2022
@github-actions github-actions Bot added the outdated PR was locked due to age label Dec 2, 2022
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Dec 2, 2022
@chenrui333 chenrui333 deleted the bump-envoy-1.23.2 branch March 12, 2023 07:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automerge-skip `brew pr-automerge` will skip this pull request bump-formula-pr PR was created using `brew bump-formula-pr` CI-requeued PR has been re-added to the queue outdated PR was locked due to age superseded PR was replaced by another PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants