From f4a9c5325b7cba8ae432466bdfb90f857da0d593 Mon Sep 17 00:00:00 2001 From: Fergus Henderson Date: Tue, 2 Apr 2024 20:50:15 +0100 Subject: [PATCH 1/5] Avoid ODR violations with flatbuffers::Verifier. (#8274) Fix "One Definition Rule" violation when using flatbuffers::Verifier with FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE defined in some compilation units and not defined in other compilation units. The fix is to make Verifier a template class, with a boolean template parameter replacing the "#ifdef" conditionals; to rename it as VerifierTemplate; and then to use "#ifdef" only for a "using" declaration that defines the original name Verifier an an alias for the instantiated template. In this way, even if FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE is defined in some compilation units and not in others, as long as clients only reference flatbuffers::Verifier in .cc files, not header files, there will be no ODR violation, since the only part whose definition varies is the "using" declaration, which does not have external linkage. There is still some possibility of clients creating ODR violations if the client header files (rather than .cc files) reference flatbuffers::Verifier. To avoid that, this change also deprecates FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE, and instead introduces flatbuffers::SizeVerifier as a public name for the template instance with the boolean parameter set to true, so that clients don't need to define the macro at all. --- include/flatbuffers/verifier.h | 108 +++++++++++++++++++++------------ 1 file changed, 70 insertions(+), 38 deletions(-) diff --git a/include/flatbuffers/verifier.h b/include/flatbuffers/verifier.h index de1146be92e..6df923be45c 100644 --- a/include/flatbuffers/verifier.h +++ b/include/flatbuffers/verifier.h @@ -23,7 +23,8 @@ namespace flatbuffers { // Helper class to verify the integrity of a FlatBuffer -class Verifier FLATBUFFERS_FINAL_CLASS { +template +class VerifierTemplate FLATBUFFERS_FINAL_CLASS { public: struct Options { // The maximum nesting of tables and vectors before we call it invalid. @@ -40,17 +41,18 @@ class Verifier FLATBUFFERS_FINAL_CLASS { bool assert = false; }; - explicit Verifier(const uint8_t *const buf, const size_t buf_len, - const Options &opts) + explicit VerifierTemplate(const uint8_t *const buf, const size_t buf_len, + const Options &opts) : buf_(buf), size_(buf_len), opts_(opts) { FLATBUFFERS_ASSERT(size_ < opts.max_size); } - // Deprecated API, please construct with Verifier::Options. - Verifier(const uint8_t *const buf, const size_t buf_len, - const uoffset_t max_depth = 64, const uoffset_t max_tables = 1000000, - const bool check_alignment = true) - : Verifier(buf, buf_len, [&] { + // Deprecated API, please construct with VerifierTemplate::Options. + VerifierTemplate(const uint8_t *const buf, const size_t buf_len, + const uoffset_t max_depth = 64, + const uoffset_t max_tables = 1000000, + const bool check_alignment = true) + : VerifierTemplate(buf, buf_len, [&] { Options opts; opts.max_depth = max_depth; opts.max_tables = max_tables; @@ -62,25 +64,25 @@ class Verifier FLATBUFFERS_FINAL_CLASS { bool Check(const bool ok) const { // clang-format off #ifdef FLATBUFFERS_DEBUG_VERIFICATION_FAILURE - if (opts_.assert) { FLATBUFFERS_ASSERT(ok); } - #endif - #ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE - if (!ok) - upper_bound_ = 0; + if (opts_.assert) { FLATBUFFERS_ASSERT(ok); } #endif // clang-format on + if (TrackVerifierBufferSize) { + if (!ok) { + upper_bound_ = 0; + } + } return ok; } // Verify any range within the buffer. bool Verify(const size_t elem, const size_t elem_len) const { - // clang-format off - #ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE + if (TrackVerifierBufferSize) { auto upper_bound = elem + elem_len; - if (upper_bound_ < upper_bound) + if (upper_bound_ < upper_bound) { upper_bound_ = upper_bound; - #endif - // clang-format on + } + } return Check(elem_len < size_ && elem <= size_ - elem_len); } @@ -210,14 +212,14 @@ class Verifier FLATBUFFERS_FINAL_CLASS { // Call T::Verify, which must be in the generated code for this type. const auto o = VerifyOffset(start); - return Check(o != 0) && - reinterpret_cast(buf_ + start + o)->Verify(*this) - // clang-format off - #ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE - && GetComputedSize() - #endif - ; - // clang-format on + if (!Check(o != 0)) return false; + if (!(reinterpret_cast(buf_ + start + o)->Verify(*this))) { + return false; + } + if (TrackVerifierBufferSize) { + if (GetComputedSize() == 0) return false; + } + return true; } template @@ -232,7 +234,8 @@ class Verifier FLATBUFFERS_FINAL_CLASS { // If there is a nested buffer, it must be greater than the min size. if (!Check(buf->size() >= FLATBUFFERS_MIN_BUFFER_SIZE)) return false; - Verifier nested_verifier(buf->data(), buf->size(), opts_); + VerifierTemplate nested_verifier( + buf->data(), buf->size(), opts_); return nested_verifier.VerifyBuffer(identifier); } @@ -286,21 +289,27 @@ class Verifier FLATBUFFERS_FINAL_CLASS { return true; } - // Returns the message size in bytes + // Returns the message size in bytes. + // + // This should only be called after first calling VerifyBuffer or + // VerifySizePrefixedBuffer. + // + // This method should only be called for VerifierTemplate instances + // where the TrackVerifierBufferSize template parameter is true, + // i.e. for SizeVerifier. For instances where TrackVerifierBufferSize + // is false, this fails at runtime or returns zero. size_t GetComputedSize() const { - // clang-format off - #ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE + if (TrackVerifierBufferSize) { uintptr_t size = upper_bound_; // Align the size to uoffset_t size = (size - 1 + sizeof(uoffset_t)) & ~(sizeof(uoffset_t) - 1); return (size > size_) ? 0 : size; - #else - // Must turn on FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE for this to work. - (void)upper_bound_; - FLATBUFFERS_ASSERT(false); - return 0; - #endif - // clang-format on + } + // Must use SizeVerifier, or (deprecated) turn on + // FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE, for this to work. + (void)upper_bound_; + FLATBUFFERS_ASSERT(false); + return 0; } std::vector *GetFlexReuseTracker() { return flex_reuse_tracker_; } @@ -323,10 +332,33 @@ class Verifier FLATBUFFERS_FINAL_CLASS { // Specialization for 64-bit offsets. template<> -inline size_t Verifier::VerifyOffset(const size_t start) const { +template<> +inline size_t VerifierTemplate::VerifyOffset( + const size_t start) const { + return VerifyOffset(start); +} +template<> +template<> +inline size_t VerifierTemplate::VerifyOffset( + const size_t start) const { return VerifyOffset(start); } +// Instance of VerifierTemplate that supports GetComputedSize(). +using SizeVerifier = VerifierTemplate; + +// The FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE build configuration macro is +// deprecated, and should not be defined, since it is easy to misuse in ways +// that result in ODR violations. Rather than using Verifier and defining +// FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE, please use SizeVerifier instead. +#ifdef FLATBUFFERS_TRACK_VERIFIER_BUFFER_SIZE // Deprecated, see above. +using Verifier = SizeVerifier; +#else +// Instance of VerifierTemplate that is slightly faster, but does not +// support GetComputedSize(). +using Verifier = VerifierTemplate; +#endif + } // namespace flatbuffers #endif // FLATBUFFERS_VERIFIER_H_ From e040f4e9756b7310bffac491b89cdb09f9fd6362 Mon Sep 17 00:00:00 2001 From: Michael Beardsworth Date: Fri, 5 Apr 2024 12:27:43 -0700 Subject: [PATCH 2/5] Improve error handling on Object API name collision. (#8275) If a schema contains a message named e.g. FooT and a message named Foo while the Object API suffix is T, then two classes with colliding names will be generated. This scenario will produce a C++ compiler error, but it's confusing. This patch moves the error to the compiler, allowing the user to more readily act to correct the issue. Co-authored-by: Michael Beardsworth --- src/idl_gen_cpp.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/idl_gen_cpp.cpp b/src/idl_gen_cpp.cpp index 301c08d2c56..ce03a5eeca5 100644 --- a/src/idl_gen_cpp.cpp +++ b/src/idl_gen_cpp.cpp @@ -467,6 +467,20 @@ class CppGenerator : public BaseGenerator { } if (opts_.generate_object_based_api) { auto nativeName = NativeName(Name(*struct_def), struct_def, opts_); + + // Check that nativeName doesn't collide the name of another struct. + for (const auto &other_struct_def : parser_.structs_.vec) { + if (other_struct_def == struct_def) { continue; } + + auto other_name = Name(*other_struct_def); + if (nativeName == other_name) { + LogCompilerError("Generated Object API type for " + + Name(*struct_def) + " collides with " + + other_name); + FLATBUFFERS_ASSERT(true); + } + } + if (!struct_def->fixed) { code_ += "struct " + nativeName + ";"; } } code_ += ""; From e6463926479bd6b330cbcf673f7e917803fd5831 Mon Sep 17 00:00:00 2001 From: Derek Bailey Date: Wed, 17 Apr 2024 16:06:26 +0000 Subject: [PATCH 3/5] `flatbuffer_builder`: Fix GetTemporaryPointer constantness --- include/flatbuffers/flatbuffer_builder.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/include/flatbuffers/flatbuffer_builder.h b/include/flatbuffers/flatbuffer_builder.h index 9a2d62541bd..9ceca8207b6 100644 --- a/include/flatbuffers/flatbuffer_builder.h +++ b/include/flatbuffers/flatbuffer_builder.h @@ -47,7 +47,8 @@ inline voffset_t FieldIndexToOffset(voffset_t field_id) { 2 * sizeof(voffset_t); // Vtable size and Object Size. size_t offset = fixed_fields + field_id * sizeof(voffset_t); FLATBUFFERS_ASSERT(offset < std::numeric_limits::max()); - return static_cast(offset);} + return static_cast(offset); +} template> const T *data(const std::vector &v) { @@ -241,7 +242,7 @@ template class FlatBufferBuilderImpl { /// called. uint8_t *ReleaseRaw(size_t &size, size_t &offset) { Finished(); - uint8_t* raw = buf_.release_raw(size, offset); + uint8_t *raw = buf_.release_raw(size, offset); Clear(); return raw; } @@ -561,7 +562,7 @@ template class FlatBufferBuilderImpl { return CreateString(str.c_str(), str.length()); } -// clang-format off + // clang-format off #ifdef FLATBUFFERS_HAS_STRING_VIEW /// @brief Store a string in the buffer, which can contain any binary data. /// @param[in] str A const string_view to copy in to the buffer. @@ -743,7 +744,7 @@ template class FlatBufferBuilderImpl { AssertScalarT(); StartVector(len); if (len > 0) { -// clang-format off + // clang-format off #if FLATBUFFERS_LITTLEENDIAN PushBytes(reinterpret_cast(v), len * sizeof(T)); #else @@ -1470,7 +1471,8 @@ T *GetMutableTemporaryPointer(FlatBufferBuilder &fbb, Offset offset) { template const T *GetTemporaryPointer(const FlatBufferBuilder &fbb, Offset offset) { - return GetMutableTemporaryPointer(fbb, offset); + return reinterpret_cast(fbb.GetCurrentBufferPointer() + + fbb.GetSize() - offset.o); } } // namespace flatbuffers From da6472013fde0992c4d70b5e289a440da3383350 Mon Sep 17 00:00:00 2001 From: Paulo Pinheiro Date: Thu, 18 Apr 2024 02:10:39 +0200 Subject: [PATCH 4/5] [Kotlin] Add workflow to release kotlin multiplatform version (#8014) Co-authored-by: Derek Bailey --- .github/workflows/release.yml | 26 +++++ kotlin/convention-plugins/build.gradle.kts | 7 ++ .../kotlin/convention.publication.gradle.kts | 95 +++++++++++++++++++ kotlin/flatbuffers-kotlin/build.gradle.kts | 1 + kotlin/settings.gradle.kts | 1 + 5 files changed, 130 insertions(+) create mode 100644 kotlin/convention-plugins/build.gradle.kts create mode 100644 kotlin/convention-plugins/src/main/kotlin/convention.publication.gradle.kts diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index e087d6f67a1..a6ff0268996 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -100,6 +100,32 @@ jobs: OSSRH_USERNAME: ${{ secrets.OSSRH_USERNAME }} OSSRH_PASSWORD: ${{ secrets.OSSRH_TOKEN }} MAVEN_GPG_PASSPHRASE: ${{ secrets.MAVEN_GPG_PASSPHRASE }} + + publish-maven-kotlin: + name: Publish Maven - Kotlin + runs-on: ubuntu-latest + defaults: + run: + working-directory: ./kotlin + steps: + - uses: actions/checkout@v3 + - name: Set up Maven Central Repository + uses: actions/setup-java@v3 + with: + java-version: '11' + distribution: 'adopt' + cache: 'maven' + server-id: ossrh + server-username: OSSRH_USERNAME + server-password: OSSRH_PASSWORD + gpg-private-key: ${{ secrets.MAVEN_GPG_PRIVATE_KEY }} + gpg-passphrase: MAVEN_GPG_PASSPHRASE # this needs to be an env var + - name: Publish Kotlin Library on Maven + run: ./gradlew publishAllPublicationsToSonatypeRepository + env: + OSSRH_USERNAME: ${{ secrets.OSSRH_USERNAME }} + OSSRH_PASSWORD: ${{ secrets.OSSRH_TOKEN }} + MAVEN_GPG_PASSPHRASE: ${{ secrets.MAVEN_GPG_PASSPHRASE }} diff --git a/kotlin/convention-plugins/build.gradle.kts b/kotlin/convention-plugins/build.gradle.kts new file mode 100644 index 00000000000..52b9cc0a8c6 --- /dev/null +++ b/kotlin/convention-plugins/build.gradle.kts @@ -0,0 +1,7 @@ +plugins { + `kotlin-dsl` +} + +repositories { + gradlePluginPortal() +} diff --git a/kotlin/convention-plugins/src/main/kotlin/convention.publication.gradle.kts b/kotlin/convention-plugins/src/main/kotlin/convention.publication.gradle.kts new file mode 100644 index 00000000000..e526279654b --- /dev/null +++ b/kotlin/convention-plugins/src/main/kotlin/convention.publication.gradle.kts @@ -0,0 +1,95 @@ +import org.gradle.api.publish.maven.MavenPublication +import org.gradle.api.tasks.bundling.Jar +import org.gradle.kotlin.dsl.`maven-publish` +import org.gradle.kotlin.dsl.signing +import java.util.* + +plugins { + `maven-publish` + signing +} + +// Stub secrets to let the project sync and build without the publication values set up +ext["signing.keyId"] = null +ext["signing.password"] = null +ext["signing.secretKeyRingFile"] = null +ext["ossrhUsername"] = null +ext["ossrhPassword"] = null + +// Grabbing secrets from local.properties file or from environment variables, which could be used on CI +val secretPropsFile = project.rootProject.file("local.properties") +if (secretPropsFile.exists()) { + secretPropsFile.reader().use { + Properties().apply { + load(it) + } + }.onEach { (name, value) -> + ext[name.toString()] = value + } +} else { + ext["signing.keyId"] = System.getenv("OSSRH_USERNAME") + ext["signing.password"] = System.getenv("OSSRH_PASSWORD") + ext["signing.secretKeyRingFile"] = System.getenv("INPUT_GPG_PRIVATE_KEY") + ext["ossrhUsername"] = System.getenv("OSSRH_USERNAME") + ext["ossrhPassword"] = System.getenv("OSSRH_PASSWORD") +} + +val javadocJar by tasks.registering(Jar::class) { + archiveClassifier.set("javadoc") +} + +fun getExtraString(name: String) = ext[name]?.toString() + +publishing { + // Configure maven central repository + repositories { + maven { + name = "sonatype" + setUrl("https://s01.oss.sonatype.org/service/local/staging/deploy/maven2/") + credentials { + username = getExtraString("ossrhUsername") + password = getExtraString("ossrhPassword") + } + } + } + + // Configure all publications + publications.withType { + // Stub javadoc.jar artifact + artifact(javadocJar.get()) + + // Provide artifacts information requited by Maven Central + pom { + name.set("Flatbuffers Kotlin") + description.set("Memory Efficient Serialization Library") + url.set("https://github.com/google/flatbuffers") + + licenses { + license { + name.set("Apache License V2.0") + url.set("https://raw.githubusercontent.com/google/flatbuffers/master/LICENSE") + } + } + developers { + developer { + id.set("https://github.com/paulovap") + name.set("Paulo Pinheiro") + email.set("paulovictor.pinheiro@gmail.com") + } + developer { + id.set("https://github.com/dbaileychess") + name.set("Derek Bailey") + email.set("dbaileychess@gmail.com") + } + } + scm { + url.set("https://github.com/google/flatbuffers") + } + } + } +} + +// Signing artifacts. Signing.* extra properties values will be used +signing { + sign(publishing.publications) +} diff --git a/kotlin/flatbuffers-kotlin/build.gradle.kts b/kotlin/flatbuffers-kotlin/build.gradle.kts index d4086537217..1b8d2242dbd 100644 --- a/kotlin/flatbuffers-kotlin/build.gradle.kts +++ b/kotlin/flatbuffers-kotlin/build.gradle.kts @@ -7,6 +7,7 @@ import org.jetbrains.kotlin.gradle.plugin.mpp.apple.XCFrameworkConfig plugins { kotlin("multiplatform") + id("convention.publication") } diff --git a/kotlin/settings.gradle.kts b/kotlin/settings.gradle.kts index 0fd19c4d22d..9b6981eebe0 100644 --- a/kotlin/settings.gradle.kts +++ b/kotlin/settings.gradle.kts @@ -1,3 +1,4 @@ rootProject.name = "flatbuffers-kotlin" +includeBuild("convention-plugins") include("flatbuffers-kotlin") include("benchmark") From 7106d86685e3395efc6df5d2ee5914fe622ed6b9 Mon Sep 17 00:00:00 2001 From: Philipp Schrader Date: Wed, 17 Apr 2024 22:06:06 -0700 Subject: [PATCH 5/5] Remove npm/rules_js dependency for C++ only use cases (#7990) When flatbuffers is being used from a project that has no use for JavaScript, users encounter an error similar to the following: ERROR: Skipping '@com_github_google_flatbuffers//:flatbuffers': error loading package '@com_github_google_flatbuffers//': Unable to find package for @npm//:defs.bzl: The repository '@npm' could not be resolved: Repository '@npm' is not defined. WARNING: Target pattern parsing failed. ERROR: error loading package '@com_github_google_flatbuffers//': Unable to find package for @npm//:defs.bzl: The repository '@npm' could not be resolved: Repository '@npm' is not defined. INFO: Elapsed time: 0.023s INFO: 0 processes. FAILED: Build did NOT complete successfully (0 packages loaded) currently loading: @com_github_google_flatbuffers// That's not ideal. Users that only care about C++ for example shouldn't be forced to deal with rules_js and friends. This patch attempts to fix that by moving the rules_js-specific things into the `ts` and `tests/ts` directories. This should allow non-JavaScript projects to ignore rules_js and friends completely. Here I basically followed the `rules_foo` example from rules_js: https://github.com/aspect-build/rules_js/tree/main/e2e/rules_foo The idea is that flatbuffers has its own npm dependencies regardless of what other projects may have. This means we should not force the user to import flatbuffers's npm dependencies. The new `ts/repositories.bzl` file is used by dependents to import flatbuffers's dependencies. They can still import their own dependencies. This cleanup allowed me to move all JavaScript-specific stuff from the top-level directory into subdirectories. There should be no changes in this patch in terms of functionality. It's just a refactor of the rules_js call sites. Users will have to add a call to the function in `ts/repositories.bzl` in their own `WORKSPACE` file. They can use `tests/ts/bazel_repository_test/WORKSPACE` as an example. Co-authored-by: Derek Bailey --- .bazelignore | 2 +- .bazelrc | 4 +- BUILD.bazel | 13 ++--- WORKSPACE | 31 ++++++++---- tests/BUILD.bazel | 10 ++++ tests/bazel_repository_test_dir/.bazelrc | 1 + tests/bazel_repository_test_dir/.gitignore | 1 + tests/bazel_repository_test_dir/BUILD | 10 ++++ tests/bazel_repository_test_dir/README.md | 8 ++++ tests/bazel_repository_test_dir/WORKSPACE | 6 +++ .../pulls_in_flatbuffers_test.cpp | 1 + ...t.sh => bazel_repository_test_template.sh} | 4 +- tests/defs.bzl | 48 +++++++++++++++++++ tests/ts/BUILD.bazel | 39 ++++++--------- .../ts/bazel_repository_test_dir/BUILD.bazel | 9 ++++ tests/ts/bazel_repository_test_dir/README.md | 8 ++++ tests/ts/bazel_repository_test_dir/WORKSPACE | 14 ++++++ .../independent_deps_test.js | 18 +++++++ .../ts/bazel_repository_test_dir/package.json | 2 +- .../bazel_repository_test_dir/pnpm-lock.yaml | 10 ++-- tests/ts/test_dir/BUILD.bazel | 2 +- ts/BUILD.bazel | 8 ++-- ts/repositories.bzl | 23 +++++++++ 23 files changed, 213 insertions(+), 59 deletions(-) create mode 100644 tests/bazel_repository_test_dir/.bazelrc create mode 100644 tests/bazel_repository_test_dir/.gitignore create mode 100644 tests/bazel_repository_test_dir/BUILD create mode 100644 tests/bazel_repository_test_dir/README.md create mode 100644 tests/bazel_repository_test_dir/WORKSPACE create mode 100644 tests/bazel_repository_test_dir/pulls_in_flatbuffers_test.cpp rename tests/{ts/bazel_repository_test.sh => bazel_repository_test_template.sh} (91%) create mode 100644 tests/defs.bzl create mode 100644 tests/ts/bazel_repository_test_dir/README.md create mode 100644 tests/ts/bazel_repository_test_dir/independent_deps_test.js create mode 100644 ts/repositories.bzl diff --git a/.bazelignore b/.bazelignore index 3c3629e647f..accce227baa 100644 --- a/.bazelignore +++ b/.bazelignore @@ -1 +1 @@ -node_modules +ts/node_modules diff --git a/.bazelrc b/.bazelrc index f9f47a74237..fe25c51e0f8 100644 --- a/.bazelrc +++ b/.bazelrc @@ -1,4 +1,4 @@ # We cannot use "common" here because the "version" command doesn't support # --deleted_packages. We need to specify it for both build and query instead. -build --deleted_packages=tests/ts/bazel_repository_test_dir -query --deleted_packages=tests/ts/bazel_repository_test_dir +build --deleted_packages=tests/bazel_repository_test_dir,tests/ts/bazel_repository_test_dir +query --deleted_packages=tests/bazel_repository_test_dir,tests/ts/bazel_repository_test_dir diff --git a/BUILD.bazel b/BUILD.bazel index b4f015a0e29..933e24ac854 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -1,5 +1,3 @@ -load("@aspect_rules_js//npm:defs.bzl", "npm_link_package") -load("@npm//:defs.bzl", "npm_link_all_packages") load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library") licenses(["notice"]) @@ -8,13 +6,6 @@ package( default_visibility = ["//visibility:public"], ) -npm_link_all_packages(name = "node_modules") - -npm_link_package( - name = "node_modules/flatbuffers", - src = "//ts:flatbuffers", -) - exports_files([ "LICENSE", "tsconfig.json", @@ -37,9 +28,13 @@ config_setting( filegroup( name = "distribution", srcs = [ + ".bazelignore", + ".npmrc", "BUILD.bazel", "WORKSPACE", "build_defs.bzl", + "package.json", + "pnpm-lock.yaml", "typescript.bzl", "//grpc/src/compiler:distribution", "//reflection:distribution", diff --git a/WORKSPACE b/WORKSPACE index e56d4ce364b..730217e472c 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -11,6 +11,21 @@ http_archive( ], ) +# Import our own version of skylib before other rule sets (e.g. rules_swift) +# has a chance to import an old version. +http_archive( + name = "bazel_skylib", + sha256 = "66ffd9315665bfaafc96b52278f57c7e2dd09f5ede279ea6d39b2be471e7e3aa", + urls = [ + "https://mirror.bazel.build/github.com/bazelbuild/bazel-skylib/releases/download/1.4.2/bazel-skylib-1.4.2.tar.gz", + "https://github.com/bazelbuild/bazel-skylib/releases/download/1.4.2/bazel-skylib-1.4.2.tar.gz", + ], +) + +load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace") + +bazel_skylib_workspace() + http_archive( name = "build_bazel_rules_apple", sha256 = "34c41bfb59cdaea29ac2df5a2fa79e5add609c71bb303b2ebb10985f93fa20e7", @@ -101,7 +116,7 @@ load("@aspect_rules_js//js:repositories.bzl", "rules_js_dependencies") rules_js_dependencies() -load("@aspect_rules_js//npm:npm_import.bzl", "npm_translate_lock", "pnpm_repository") +load("@aspect_rules_js//npm:npm_import.bzl", "pnpm_repository") pnpm_repository(name = "pnpm") @@ -129,17 +144,13 @@ nodejs_register_toolchains( node_version = DEFAULT_NODE_VERSION, ) -npm_translate_lock( - name = "npm", - npmrc = "//:.npmrc", - pnpm_lock = "//:pnpm-lock.yaml", - # Set this to True when the lock file needs to be updated, commit the - # changes, then set to False again. - update_pnpm_lock = False, - verify_node_modules_ignored = "//:.bazelignore", +load("@com_github_google_flatbuffers//ts:repositories.bzl", "flatbuffers_npm") + +flatbuffers_npm( + name = "flatbuffers_npm", ) -load("@npm//:repositories.bzl", "npm_repositories") +load("@flatbuffers_npm//:repositories.bzl", "npm_repositories") npm_repositories() diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel index f306f7ec82f..6313ed1b0a4 100644 --- a/tests/BUILD.bazel +++ b/tests/BUILD.bazel @@ -1,9 +1,14 @@ load("@aspect_bazel_lib//lib:copy_to_bin.bzl", "copy_to_bin") load("@rules_cc//cc:defs.bzl", "cc_test") load("//:build_defs.bzl", "DEFAULT_FLATC_ARGS", "flatbuffer_cc_library") +load(":defs.bzl", "flatbuffers_as_external_repo_test") package(default_visibility = ["//visibility:private"]) +exports_files([ + "bazel_repository_test_template.sh", +]) + # rules_js works around various JS tooling limitations by copying everything # into the output directory. Make the test data available to the tests this way. copy_to_bin( @@ -264,3 +269,8 @@ flatbuffer_cc_library( name = "alignment_test_cc_fbs", srcs = ["alignment_test.fbs"], ) + +flatbuffers_as_external_repo_test( + name = "bazel_repository_test", + directory = "bazel_repository_test_dir", +) diff --git a/tests/bazel_repository_test_dir/.bazelrc b/tests/bazel_repository_test_dir/.bazelrc new file mode 100644 index 00000000000..78003332b01 --- /dev/null +++ b/tests/bazel_repository_test_dir/.bazelrc @@ -0,0 +1 @@ +build --symlink_prefix=/ diff --git a/tests/bazel_repository_test_dir/.gitignore b/tests/bazel_repository_test_dir/.gitignore new file mode 100644 index 00000000000..ac51a054d2d --- /dev/null +++ b/tests/bazel_repository_test_dir/.gitignore @@ -0,0 +1 @@ +bazel-* diff --git a/tests/bazel_repository_test_dir/BUILD b/tests/bazel_repository_test_dir/BUILD new file mode 100644 index 00000000000..36fe6bd39db --- /dev/null +++ b/tests/bazel_repository_test_dir/BUILD @@ -0,0 +1,10 @@ +# This test doesn't actually make use of the flatbuffers library. It's just +# here to make sure we can link the library properly when it comes from an +# external repository. You're welcome to expand this test to do more. +cc_test( + name = "pulls_in_flatbuffers_test", + srcs = ["pulls_in_flatbuffers_test.cpp"], + deps = [ + "@com_github_google_flatbuffers//:flatbuffers", + ], +) diff --git a/tests/bazel_repository_test_dir/README.md b/tests/bazel_repository_test_dir/README.md new file mode 100644 index 00000000000..6f7c34a3601 --- /dev/null +++ b/tests/bazel_repository_test_dir/README.md @@ -0,0 +1,8 @@ +This directory is not intended to be used independently of the flatbuffers +repository. Instead, this whole directory serves as a unit test for the +C++ integration in the flatbuffers repo. + +Run this test from the top-level of the flatbuffers repo. +```console +$ bazel test //tests:bazel_repository_test +``` diff --git a/tests/bazel_repository_test_dir/WORKSPACE b/tests/bazel_repository_test_dir/WORKSPACE new file mode 100644 index 00000000000..3adbfc61fc9 --- /dev/null +++ b/tests/bazel_repository_test_dir/WORKSPACE @@ -0,0 +1,6 @@ +workspace(name = "bazel_repository_test") + +local_repository( + name = "com_github_google_flatbuffers", + path = "../../", +) diff --git a/tests/bazel_repository_test_dir/pulls_in_flatbuffers_test.cpp b/tests/bazel_repository_test_dir/pulls_in_flatbuffers_test.cpp new file mode 100644 index 00000000000..76e8197013a --- /dev/null +++ b/tests/bazel_repository_test_dir/pulls_in_flatbuffers_test.cpp @@ -0,0 +1 @@ +int main() { return 0; } diff --git a/tests/ts/bazel_repository_test.sh b/tests/bazel_repository_test_template.sh similarity index 91% rename from tests/ts/bazel_repository_test.sh rename to tests/bazel_repository_test_template.sh index 94e115c747e..5c7736fcf1a 100755 --- a/tests/ts/bazel_repository_test.sh +++ b/tests/bazel_repository_test_template.sh @@ -1,7 +1,7 @@ #!/bin/bash # This test makes sure that a separate repository can import the flatbuffers -# repository and use it in their JavaScript code. +# repository and use it in their code. # --- begin runfiles.bash initialization v3 --- # Copy-pasted from the Bazel Bash runfiles library v3. @@ -24,6 +24,6 @@ fi export PATH="$(dirname "${BAZEL_BIN}"):${PATH}" -cd tests/ts/bazel_repository_test_dir/ +cd {{REPOSITORY_DIR}} bazel test //... diff --git a/tests/defs.bzl b/tests/defs.bzl new file mode 100644 index 00000000000..033d2303093 --- /dev/null +++ b/tests/defs.bzl @@ -0,0 +1,48 @@ +"""Helper macros and rules for tests.""" + +load("@bazel_skylib//lib:paths.bzl", "paths") +load("@bazel_skylib//rules:expand_template.bzl", "expand_template") + +def flatbuffers_as_external_repo_test(name, directory): + """Run all tests in a bazel workspace that imports flatbuffers as an external repository. + + Args: + name: The name of the test target. + directory: The directory in which the bazel workspace is located. This is the directory + that imports flatbuffers as an external repository. + """ + expand_template( + name = name + "__template_expansion", + out = name + ".sh", + substitutions = { + "{{REPOSITORY_DIR}}": paths.join(native.package_name(), directory), + }, + template = "//tests:bazel_repository_test_template.sh", + ) + + native.sh_test( + name = name, + srcs = [":%s.sh" % name], + data = [ + "//:distribution", + "@bazel_linux_x86_64//file", + ] + native.glob( + [ + directory + "/**/*", + ], + exclude = [ + directory + "/bazel-*/**", + ], + ), + tags = [ + # Since we have bazel downloading external repositories inside this + # test, we need to give it access to the internet. + "requires-network", + ], + # We only have x86_64 Linux bazel exposed so restrict the test to that. + target_compatible_with = [ + "@platforms//cpu:x86_64", + "@platforms//os:linux", + ], + deps = ["@bazel_tools//tools/bash/runfiles"], + ) diff --git a/tests/ts/BUILD.bazel b/tests/ts/BUILD.bazel index 82635450b7e..ee97f8fde70 100644 --- a/tests/ts/BUILD.bazel +++ b/tests/ts/BUILD.bazel @@ -1,8 +1,19 @@ load("@aspect_rules_js//js:defs.bzl", "js_test") +load("@aspect_rules_js//npm:defs.bzl", "npm_link_package") load("//:typescript.bzl", "flatbuffer_ts_library") +load("//tests:defs.bzl", "flatbuffers_as_external_repo_test") package(default_visibility = ["//visibility:private"]) +# This is a copy of //ts:node_modules/flatbuffers. The rules_js-based tests +# require this target to live in the same or a parent package. Since we don't +# want to put rules_js targets in the root package, we create a copy here. +npm_link_package( + name = "node_modules/flatbuffers", + src = "//ts:flatbuffers", + root_package = "tests/ts", +) + flatbuffer_ts_library( name = "typescript_ts_fbs", srcs = ["typescript_keywords.fbs"], @@ -37,8 +48,8 @@ TEST_COMPLEX_ARRAYS_DATA = glob([ chdir = package_name(), data = data + [ "package.json", - "//:node_modules/flatbuffers", "//tests:test_data_copied_to_bin", + "//tests/ts:node_modules/flatbuffers", ], entry_point = "%s.js" % test, ) for test, data in ( @@ -50,29 +61,7 @@ TEST_COMPLEX_ARRAYS_DATA = glob([ ("JavaScriptComplexArraysTest", TEST_COMPLEX_ARRAYS_DATA), )] -sh_test( +flatbuffers_as_external_repo_test( name = "bazel_repository_test", - srcs = ["bazel_repository_test.sh"], - data = [ - "//:distribution", - "@bazel_linux_x86_64//file", - ] + glob( - [ - "bazel_repository_test_dir/**/*", - ], - exclude = [ - "bazel_repository_test_dir/bazel-*/**", - ], - ), - tags = [ - # Since we have bazel downloading external repositories inside this - # test, we need to give it access to the internet. - "requires-network", - ], - # We only have x86_64 Linux bazel exposed so restrict the test to that. - target_compatible_with = [ - "@platforms//cpu:x86_64", - "@platforms//os:linux", - ], - deps = ["@bazel_tools//tools/bash/runfiles"], + directory = "bazel_repository_test_dir", ) diff --git a/tests/ts/bazel_repository_test_dir/BUILD.bazel b/tests/ts/bazel_repository_test_dir/BUILD.bazel index f6b01c5ec30..8b97909d34f 100644 --- a/tests/ts/bazel_repository_test_dir/BUILD.bazel +++ b/tests/ts/bazel_repository_test_dir/BUILD.bazel @@ -30,3 +30,12 @@ js_test( ], entry_point = "import_test.js", ) + +js_test( + name = "independent_deps_test", + data = [ + "package.json", + ":node_modules/lodash", + ], + entry_point = "independent_deps_test.js", +) diff --git a/tests/ts/bazel_repository_test_dir/README.md b/tests/ts/bazel_repository_test_dir/README.md new file mode 100644 index 00000000000..a8bf76e1831 --- /dev/null +++ b/tests/ts/bazel_repository_test_dir/README.md @@ -0,0 +1,8 @@ +This directory is not intended to be used independently of the flatbuffers +repository. Instead, this whole directory serves as a unit test for the +`rules_js` integration in the flatbuffers repo. + +Run this test from the top-level of the flatbuffers repo. +```console +$ bazel test //tests/ts:bazel_repository_test +``` diff --git a/tests/ts/bazel_repository_test_dir/WORKSPACE b/tests/ts/bazel_repository_test_dir/WORKSPACE index f7ef4541f38..27fb8d6a468 100644 --- a/tests/ts/bazel_repository_test_dir/WORKSPACE +++ b/tests/ts/bazel_repository_test_dir/WORKSPACE @@ -46,8 +46,12 @@ nodejs_register_toolchains( npm_translate_lock( name = "npm", + data = [ + "//:package.json", + ], npmrc = "//:.npmrc", pnpm_lock = "//:pnpm-lock.yaml", + update_pnpm_lock = False, verify_node_modules_ignored = "//:.bazelignore", ) @@ -69,3 +73,13 @@ esbuild_register_toolchains( name = "esbuild", esbuild_version = LATEST_VERSION, ) + +load("@com_github_google_flatbuffers//ts:repositories.bzl", "flatbuffers_npm") + +flatbuffers_npm( + name = "flatbuffers_npm", +) + +load("@flatbuffers_npm//:repositories.bzl", flatbuffers_npm_repositories = "npm_repositories") + +flatbuffers_npm_repositories() diff --git a/tests/ts/bazel_repository_test_dir/independent_deps_test.js b/tests/ts/bazel_repository_test_dir/independent_deps_test.js new file mode 100644 index 00000000000..02f72b4867e --- /dev/null +++ b/tests/ts/bazel_repository_test_dir/independent_deps_test.js @@ -0,0 +1,18 @@ +// This test has nothing to do with flatbuffers. It only exists to validate +// that other projects can use their own set of dependencies without having to +// explicitly pull in flatbuffers's dependencies. +// +// We pick lodash here not for any particular reason. It could be any package, +// really. I chose it because it's a relatively simple package. + +import assert from 'node:assert/strict' + +import _ from 'lodash' + +function main() { + console.log(_); + assert.deepStrictEqual(_.defaults({ 'a': 1 }, { 'a': 3, 'b': 2 }), { 'a': 1, 'b': 2 }); + assert.deepStrictEqual(_.partition([1, 2, 3, 4], n => n % 2), [[1, 3], [2, 4]]); +} + +main(); diff --git a/tests/ts/bazel_repository_test_dir/package.json b/tests/ts/bazel_repository_test_dir/package.json index 7bab70109d7..2988b7e80f3 100644 --- a/tests/ts/bazel_repository_test_dir/package.json +++ b/tests/ts/bazel_repository_test_dir/package.json @@ -3,6 +3,6 @@ "type": "module", "private": true, "devDependencies": { - "@types/node": "18.15.11" + "lodash": "4.17.21" } } diff --git a/tests/ts/bazel_repository_test_dir/pnpm-lock.yaml b/tests/ts/bazel_repository_test_dir/pnpm-lock.yaml index 331070a317b..09183910901 100644 --- a/tests/ts/bazel_repository_test_dir/pnpm-lock.yaml +++ b/tests/ts/bazel_repository_test_dir/pnpm-lock.yaml @@ -1,12 +1,12 @@ lockfileVersion: '6.0' devDependencies: - '@types/node': - specifier: 18.15.11 - version: 18.15.11 + lodash: + specifier: 4.17.21 + version: 4.17.21 packages: - /@types/node@18.15.11: - resolution: {integrity: sha512-E5Kwq2n4SbMzQOn6wnmBjuK9ouqlURrcZDVfbo9ftDDTFt3nk7ZKK4GMOzoYgnpQJKcxwQw+lGaBvvlMo0qN/Q==} + /lodash@4.17.21: + resolution: {integrity: sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==} dev: true diff --git a/tests/ts/test_dir/BUILD.bazel b/tests/ts/test_dir/BUILD.bazel index 6026d9ff569..fdd54ded39b 100644 --- a/tests/ts/test_dir/BUILD.bazel +++ b/tests/ts/test_dir/BUILD.bazel @@ -20,7 +20,7 @@ js_test( data = [ "package.json", ":include_ts_fbs", - "//:node_modules/flatbuffers", + "//tests/ts:node_modules/flatbuffers", ], entry_point = "import_test.js", ) diff --git a/ts/BUILD.bazel b/ts/BUILD.bazel index 9bd9f4be31a..804faea619a 100644 --- a/ts/BUILD.bazel +++ b/ts/BUILD.bazel @@ -1,17 +1,21 @@ load("@aspect_rules_js//npm:defs.bzl", "npm_package") load("@aspect_rules_ts//ts:defs.bzl", "ts_project") +load("@flatbuffers_npm//:npm_link_all_packages.bzl", "npm_link_all_packages") filegroup( name = "distribution", srcs = [ "BUILD.bazel", "compile_flat_file.sh", + "repositories.bzl", ] + glob([ "*.ts", ]), visibility = ["//visibility:public"], ) +npm_link_all_packages(name = "node_modules") + # Add an index to emulate the top-level package.json's "main" entry. genrule( name = "generate_index.ts", @@ -48,9 +52,7 @@ ts_project( }, visibility = ["//visibility:public"], deps = [ - # Because the main repository instantiates the @npm repository, we need - # to depend on the main repository's node import. - "@//:node_modules/@types/node", + ":node_modules/@types/node", ], ) diff --git a/ts/repositories.bzl b/ts/repositories.bzl new file mode 100644 index 00000000000..284d3d2ac72 --- /dev/null +++ b/ts/repositories.bzl @@ -0,0 +1,23 @@ +"""WORKSPACE macro to load flatbuffers's npm package list.""" + +load("@aspect_rules_js//npm:npm_import.bzl", _npm_translate_lock = "npm_translate_lock") + +def flatbuffers_npm(name): + _npm_translate_lock( + name = name, + npmrc = "@com_github_google_flatbuffers//:.npmrc", + pnpm_lock = "@com_github_google_flatbuffers//:pnpm-lock.yaml", + # Override the Bazel package where pnpm-lock.yaml is located and link + # to the specified package instead. + root_package = "ts", + # Set this to True when the lock file needs to be updated, commit the + # changes, then set to False again. + # Alternatively, run: + # $ bazel run -- @pnpm//:pnpm --dir $PWD install --lockfile-only + update_pnpm_lock = False, + verify_node_modules_ignored = "@com_github_google_flatbuffers//:.bazelignore", + defs_bzl_filename = "npm_link_all_packages.bzl", + data = [ + "@com_github_google_flatbuffers//:package.json", + ], + )