Investigate getting rid of floating patch for `secondary_source` support.

[ry]

Using 9.2-lkgr (specifically https://github.com/v8/v8/commit/84e9cc869c13a28dec61aea4dcac427f0e940e6c), we get an error during gn gen

ERROR at //gni/v8.gni:89:25: Duplicate build argument declaration.
  cppgc_is_standalone = false
                        ^----
Here you're declaring an argument that was already declared elsewhere.
You can only declare each argument once in the entire build so there is one
canonical place for documentation and the default value. Either move this
argument to the build config file (for visibility everywhere) or to a .gni file
that you "import" from the files where you need it (preferred).
See //v8/gni/v8.gni:89:25: Previous declaration.
  cppgc_is_standalone = false
                        ^----
See also "gn help buildargs" for more on how build arguments work.
See //third_party/googletest/BUILD.gn:5:1: whence it was imported.
import("../../gni/v8.gni")
^------------------------
See //testing/gmock/BUILD.gn:21:5: which caused the file to be included.
    "//third_party/googletest:gmock_config",
    ^--------------------------------------

The problem seems to be that we're importing v8.gni twice. The import("../../gni/v8.gni") in v8/third_party/googletest/BUILD.gn was recently added https://github.com/v8/v8/commit/7f9d7f0e40c57a15a4c052f92321f1508cd7d917.

I suspect this problem is because we use V8 as a secondary_source and somehow GN doesn't recognize that v8.gni was already imported? https://github.com/denoland/rusty_v8/blob/be1c2073ec1675970b4358016dc8a98ce5356f97/.gn#L14-L17

I have tried upgrading gn, upgrading //build, upgrading //buildtools - commonly how we resolve build failures in V8. None of these change the error message.

When I revert https://github.com/v8/v8/commit/7f9d7f0e40c57a15a4c052f92321f1508cd7d917, the build works correctly. I'm not sure how we can workaround this.

cc @MayaLekova ref https://github.com/denoland/rusty_v8/pull/692

[MayaLekova]

Sorry for the error you're experiencing. I'm curious though why other BUILD.gn files that do import("../../gni/v8.gni") don't cause the same issue, such as v8/tools/BUILD.gn for instance. I agree with your analysis that probably using secondary_source triggers the issue, as normally importing the same .gni file twice shouldn't have any effect.

Is it a viable temporary solution to have a floating patch with the faulty CL reverted? In the meantime, I'm trying to find a person more familiar with GN to give us a hint what would be a proper way to solve this (couldn't find any public forum about GN usage).

[ry]

@MayaLekova Thank you for the reply and asking around for us. Yes, if necessary, we can float the revert patch - but that causes other complications.

[MayaLekova]

I got a reply from a developer on the GN team:

I don't think there was really an intended behavior here, I'm sort of surprised that secondary_source works at all for .gni files.

The intended use of secondary_source is for when you're pulling in a third_party repository and want to add a BUILD.gn file in the "natural" place without modifying the upstream repository. So pulling a repository into secondary_source and then using its BUILD.gn file doesn't make sense to me. I would suggest that they stop doing this (though I assume there's also a reason I just don't understand).

I guess not using secondary_source is not really an option for you, but just wanted to share this piece of advice.

If the file is always imported as //v8/v8.gni I think it would probably avoid the problem. I assume some files are using relative paths which is what is triggering the issue.

Then they pointed out that the GN team accepts patches, e.g. if you know what the exact issue is and are able to make the path handling more consistent, that would be welcome. I know this doesn't really answer your original issue, but maybe you'll find some of the tips useful. Please let me know if I could be of further help!

[ry]

I'll float the revert in this branch https://github.com/denoland/v8/commits/9.2-lkgr-revert-7f9d7f0

This is not a sustainable solution - but will work for the time being https://github.com/denoland/rusty_v8/pull/700

[ry]

I've landed #700 which works around this issue by reverting v8/v8@7f9d7f0.

This is a fine stop-gap, but in the long-run it is untenable for us to float patches like this.

[zuon]

@ry I got the same error when building rusty_v8 0.25.2. Do I need works around this issue by reverting v8/v8@7f9d7f0

Error info: [rusty_v8 0.25.2] cargo:rerun-if-changed=src/binding.cc [rusty_v8 0.25.2] using Chromiums clang [rusty_v8 0.25.2] clang_base_path /opt/rusty_v8/target/aarch64-linux-android/debug/clang [rusty_v8 0.25.2] Downloading https://commondatastorage.googleapis.com/chromium-browser-clang/Linux_x64/clang-llvmorg-12-init-11060-g118c3f3c-1.tgz .......... Done. [rusty_v8 0.25.2] cargo:warning=Not using sccache or ccache [rusty_v8 0.25.2] The current directory is /opt/rusty_v8 [rusty_v8 0.25.2] gn gen --root=/opt/rusty_v8 /opt/rusty_v8/target/aarch64-linux-android/debug/gn_out [rusty_v8 0.25.2] running: "gn" "--root=/opt/rusty_v8" "gen" "/opt/rusty_v8/target/aarch64-linux-android/debug/gn_out" "--args=is_debug=true clang_base_path="/opt/rusty_v8/target/aarch64-linux-android/debug/clang" target_os="android" target_cpu="arm64"" [rusty_v8 0.25.2] ERROR at //gni/v8.gni:89:25: Duplicate build argument declaration. [rusty_v8 0.25.2] cppgc_is_standalone = false [rusty_v8 0.25.2] ^---- [rusty_v8 0.25.2] Here you're declaring an argument that was already declared elsewhere. [rusty_v8 0.25.2] You can only declare each argument once in the entire build so there is one [rusty_v8 0.25.2] canonical place for documentation and the default value. Either move this [rusty_v8 0.25.2] argument to the build config file (for visibility everywhere) or to a .gni file [rusty_v8 0.25.2] that you "import" from the files where you need it (preferred). [rusty_v8 0.25.2] See //v8/gni/v8.gni:89:25: Previous declaration. [rusty_v8 0.25.2] cppgc_is_standalone = false [rusty_v8 0.25.2] ^---- [rusty_v8 0.25.2] See also "gn help buildargs" for more on how build arguments work. [rusty_v8 0.25.2] See //third_party/googletest/BUILD.gn:5:1: whence it was imported. [rusty_v8 0.25.2] import("../../gni/v8.gni") [rusty_v8 0.25.2] ^------------------------ [rusty_v8 0.25.2] See //testing/gmock/BUILD.gn:18:12: which caused the file to be included. [rusty_v8 0.25.2] deps = [ "//third_party/googletest:gmock" ] [rusty_v8 0.25.2] ^------------------------------- [rusty_v8 0.25.2] thread 'main' panicked at ' [rusty_v8 0.25.2] command did not execute successfully, got: exit code: 1 [rusty_v8 0.25.2] [rusty_v8 0.25.2] build script failed, must exit now', build.rs:566:3 [rusty_v8 0.25.2] note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

[espindola]

Is this still relevant? Looks like the v8 submodule is at 9.6.180.7.

[lucacasonato]

We did eventually upgrade by floating a patch. @ry do you want to keep this open to track removing the floated patch? If so, we should rename the issue.