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

deps: update ada to 3.1.2 #57208

Closed
wants to merge 1 commit into from
Closed

Conversation

nodejs-github-bot
Copy link
Collaborator

This is an automated update of ada to 3.1.2.

@nodejs-github-bot nodejs-github-bot added the dependencies Pull requests that update a dependency file. label Feb 25, 2025
@nodejs-github-bot
Copy link
Collaborator Author

Review requested:

  • @nodejs/security-wg
  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 25, 2025
@anonrig
Copy link
Member

anonrig commented Feb 25, 2025

../deps/ada/ada.h:9483:48: error: comparison of integers of different signs: 'const value_type' (aka 'const char') and 'uint32_t' (aka 'unsigned int') [-Werror,-Wsign-compare]
 9483 |   if (!token->value.empty() && token->value[0] != value) {
      |                                ~~~~~~~~~~~~~~~ ^  ~~~~~
      ```

@lemire it seems the latest release doesn't fix this... any ideas?

@anonrig
Copy link
Member

anonrig commented Feb 25, 2025

@richardlau @nodejs/build @nodejs/url any suggestions on how to reproduce this on github.com/ada-url/ada?

@richardlau
Copy link
Member

@richardlau @nodejs/build @nodejs/url any suggestions on how to reproduce this on github.com/ada-url/ada?

Have you tried the GH Ubuntu 24.04 runner with clang (apparently 18.1.3) and -Werror? Or macOS with Xcode 16.1 and -Werror?

@anonrig
Copy link
Member

anonrig commented Feb 25, 2025

Have you tried the GH Ubuntu 24.04 runner with clang (apparently 18.1.3) and -Werror? Or macOS with Xcode 16.1 and -Werror?

I can't seem to reproduce.

@bricss
Copy link

bricss commented Feb 25, 2025

AI 🤖 suggests 💡 an explicit type cast to fix the error 🫠 or change token value type 🤔

@anonrig
Copy link
Member

anonrig commented Feb 25, 2025

AI 🤖 suggests 💡 an explicit type cast to fix the error 🫠 or change token value type 🤔

Without a reproduction on Ada's CI, it's extremely likely to regress.

@richardlau
Copy link
Member

@anonrig re.

In file included from ../src/node_url_pattern.h:6:
../deps/ada/ada.h:9483:48: error: comparison of integers of different signs: 'const value_type' (aka 'const char') and 'uint32_t' (aka 'unsigned int') [-Werror,-Wsign-compare]
 9483 |   if (!token->value.empty() && token->value[0] != value) {
      |                                ~~~~~~~~~~~~~~~ ^  ~~~~~

-Wsign-compare is being turned on by -Wextra which is set

'cflags': [ '-Wall', '-Wextra', '-Wno-unused-parameter', ],

and turned into an error by -Werror (configure --error-on-warn).

With ada:

root@25a538c9f180:/tmp/ada# git status
On branch main
Your branch is up to date with 'origin/main'.

nothing to commit, working tree clean
root@25a538c9f180:/tmp/ada# git rev-parse HEAD
88f992e29de7947c3312f0decd5ba33915e1c881
root@25a538c9f180:/tmp/ada# export CXX=g++-12
root@25a538c9f180:/tmp/ada# export CXXFLAGS="-Werror -Wextra"
root@25a538c9f180:/tmp/ada# cmake -D ADA_TESTING=ON -DBUILD_SHARED_LIBS=OFF -G Ninja -B build
-- The C compiler identification is GNU 11.4.0
-- The CXX compiler identification is GNU 12.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/g++-12 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- No build type selected, default to Release
-- Compiler ID : GNU
-- CMAKE_BUILD_TYPE : Release
-- Assuming GCC-like compiler.
-- CPM: Adding package [email protected] (v1.15.2)
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- CPM: Adding package [email protected] (v3.10.1)
-- Looking for fork
-- Looking for fork - found
-- Looking for wait
-- Looking for wait - found
-- Adding -Og to compile flag
-- The tests are enabled.
-- Found Python3: /usr/bin/python3.10 (found version "3.10.12") found components: Interpreter
-- Python found, we are going to amalgamate.py.
-- Configuring done
-- Generating done
-- Build files have been written to: /tmp/ada/build
root@25a538c9f180:/tmp/ada# cmake --build build -j=4
[11/31] Building CXX object tests/CMakeFiles/wpt_urlpattern_tests.dir/wpt_urlpattern_tests.cpp.o
FAILED: tests/CMakeFiles/wpt_urlpattern_tests.dir/wpt_urlpattern_tests.cpp.o
/usr/bin/g++-12 -DADA_TESTING=1 -DADA_USE_UNSAFE_STD_REGEX_PROVIDER=ON -DSIMDJSON_THREADS_ENABLED=1 -I/tmp/ada/include -I/tmp/ada/build/_deps/simdjson-src/include -isystem /tmp/ada/build/_deps/gtest-src/googletest/include -isystem /tmp/ada/build/_deps/gtest-src/googletest -Werror -Wextra -O3 -DNDEBUG -fPIC -std=c++20 -MD -MT tests/CMakeFiles/wpt_urlpattern_tests.dir/wpt_urlpattern_tests.cpp.o -MF tests/CMakeFiles/wpt_urlpattern_tests.dir/wpt_urlpattern_tests.cpp.o.d -o tests/CMakeFiles/wpt_urlpattern_tests.dir/wpt_urlpattern_tests.cpp.o -c /tmp/ada/tests/wpt_urlpattern_tests.cpp
In file included from /tmp/ada/include/ada.h:35,
                 from /tmp/ada/tests/wpt_urlpattern_tests.cpp:8:
/tmp/ada/include/ada/url_pattern_helpers-inl.h: In instantiation of 'constexpr bool ada::url_pattern_helpers::constructor_string_parser<regex_provider>::is_non_special_pattern_char(std::size_t, uint32_t) const [with regex_provider = ada::url_pattern_regex::std_regex_provider; std::size_t = long unsigned int; uint32_t = unsigned int]':
/tmp/ada/include/ada/url_pattern_helpers-inl.h:58:10:   required from 'constexpr bool ada::url_pattern_helpers::constructor_string_parser<regex_provider>::is_hash_prefix() [with regex_provider = ada::url_pattern_regex::std_regex_provider]'
/tmp/ada/include/ada/url_pattern_helpers-inl.h:864:34:   required from 'static tl::expected<ada::url_pattern_init, ada::errors> ada::url_pattern_helpers::constructor_string_parser<regex_provider>::parse(std::string_view) [with regex_provider = ada::url_pattern_regex::std_regex_provider; std::string_view = std::basic_string_view<char>]'
/tmp/ada/include/ada/parser-inl.h:28:78:   required from 'tl::expected<ada::url_pattern<regex_provider>, ada::errors> ada::parser::parse_url_pattern_impl(std::variant<std::basic_string_view<char, std::char_traits<char> >, ada::url_pattern_init>, const std::string_view*, const ada::url_pattern_options*) [with regex_provider = ada::url_pattern_regex::std_regex_provider; std::string_view = std::basic_string_view<char>]'
/tmp/ada/include/ada/implementation-inl.h:21:56:   required from 'tl::expected<ada::url_pattern<regex_provider>, ada::errors> ada::parse_url_pattern(std::variant<std::basic_string_view<char, std::char_traits<char> >, url_pattern_init>, const std::string_view*, const url_pattern_options*) [with regex_provider = url_pattern_regex::std_regex_provider; std::string_view = std::basic_string_view<char>]'
/tmp/ada/tests/wpt_urlpattern_tests.cpp:21:50:   required from here
/tmp/ada/include/ada/url_pattern_helpers-inl.h:105:48: error: comparison of integer expressions of different signedness: 'const __gnu_cxx::__alloc_traits<std::allocator<char>, char>::value_type' {aka 'const char'} and 'uint32_t' {aka 'unsigned int'} [-Werror=sign-compare]
  105 |   if (!token->value.empty() && token->value[0] != value) {
      |                                ~~~~~~~~~~~~~~~~^~~~~~~~
/tmp/ada/tests/wpt_urlpattern_tests.cpp: In function 'std::variant<ada::url_pattern_init, ada::url_pattern_options> parse_init(simdjson::fallback::ondemand::object&)':
/tmp/ada/tests/wpt_urlpattern_tests.cpp:151:19: error: 'value_true' may be used uninitialized [-Werror=maybe-uninitialized]
  151 |       return ada::url_pattern_options{.ignore_case = value_true};
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/ada/tests/wpt_urlpattern_tests.cpp:149:12: note: 'value_true' was declared here
  149 |       bool value_true;
      |            ^~~~~~~~~~
cc1plus: all warnings being treated as errors
[14/31] Building CXX object tests/CMakeFiles/basic_tests.dir/basic_tests.cpp.o
ninja: build stopped: subcommand failed.
root@25a538c9f180:/tmp/ada#

@richardlau
Copy link
Member

FWIW you can see in the GHA output all of the -W... options being passed to the compiler, e.g. https://github.com/nodejs/node/actions/runs/13528684029/job/37805455311?pr=57208#step:6:6543

  sccache clang++ -o /home/runner/work/node/node/node/out/Release/obj.target/libnode/src/node_url_pattern.o ../src/node_url_pattern.cc '-D_GLIBCXX_USE_CXX11_ABI=1' '-DNODE_OPENSSL_CONF_NAME=nodejs_conf' '-DICU_NO_USER_DATA_OVERRIDE' '-D__STDC_FORMAT_MACROS' '-DOPENSSL_NO_PINSHARED' '-DOPENSSL_THREADS' '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' '-DNODE_WANT_INTERNALS=1' '-DV8_DEPRECATION_WARNINGS=1' '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' '-DSQLITE_ENABLE_SESSION' '-DNODE_USE_NODE_CODE_CACHE=1' '-DHAVE_INSPECTOR=1' '-DNODE_ENABLE_LARGE_CODE_PAGES=1' '-D__POSIX__' '-DNODE_USE_V8_PLATFORM=1' '-DNODE_V8_SHARED_RO_HEAP' '-DNODE_HAVE_I18N_SUPPORT=1' '-DNODE_BUNDLED_ZLIB' '-DHAVE_OPENSSL=1' '-DOPENSSL_API_COMPAT=0x10100000L' '-DHAVE_AMARO=1' '-DXXH_NAMESPACE=ZSTD_' '-DZSTD_MULTITHREAD' '-DZSTD_DISABLE_ASM' '-DUCONFIG_NO_SERVICE=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=1' '-DUCONFIG_NO_BREAK_ITERATION=0' '-D_LARGEFILE_SOURCE' '-D_FILE_OFFSET_BITS=64' '-D_POSIX_C_SOURCE=200112' '-DNGHTTP2_STATICLIB' '-DNDEBUG' '-DOPENSSL_USE_NODELETE' '-DL_ENDIAN' '-DOPENSSL_BUILDING_OPENSSL' '-DAES_ASM' '-DBSAES_ASM' '-DCMLL_ASM' '-DECP_NISTZ256_ASM' '-DGHASH_ASM' '-DKECCAK1600_ASM' '-DMD5_ASM' '-DOPENSSL_BN_ASM_GF2m' '-DOPENSSL_BN_ASM_MONT' '-DOPENSSL_BN_ASM_MONT5' '-DOPENSSL_CPUID_OBJ' '-DOPENSSL_IA32_SSE2' '-DPADLOCK_ASM' '-DPOLY1305_ASM' '-DRC4_ASM' '-DSHA1_ASM' '-DSHA256_ASM' '-DSHA512_ASM' '-DVPAES_ASM' '-DWHIRLPOOL_ASM' '-DX25519_ASM' '-DOPENSSL_PIC' -I../src -I../deps/postject -I/home/runner/work/node/node/node/out/Release/obj/gen -I/home/runner/work/node/node/node/out/Release/obj/gen/include -I/home/runner/work/node/node/node/out/Release/obj/gen/src -I../deps/googletest/include -I../deps/histogram/src -I../deps/histogram/include -I../deps/nbytes/include -I../deps/inspector_protocol -I/home/runner/work/node/node/node/out/Release/obj/gen/inspector-generated-output-root/include -I../deps/ncrypto -I../deps/v8/include -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common -I../deps/zlib -I../deps/llhttp/include -I../deps/cares/include -I../deps/uv/include -I../deps/uvwasi/include -I../deps/nghttp2/lib/includes -I../deps/ada -I../deps/simdjson -I../deps/simdutf -I../deps/brotli/c/include -I../deps/sqlite -I../deps/zstd/lib -I../deps/openssl/openssl/include -I../deps/openssl/openssl/crypto/include -I../deps/openssl/config/archs/linux-x86_64/asm_avx2/include -I../deps/openssl/config/archs/linux-x86_64/asm_avx2  -Wall -Wextra -Wno-unused-parameter -pthread -Wall -Wextra -Wno-unused-parameter -Werror=unused-result -Werror -Werror=undefined-inline -Werror=extra-semi -Werror=ctad-maybe-unsupported -Wno-error=deprecated-declarations -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -fno-strict-aliasing -std=gnu++20 -MMD -MF /home/runner/work/node/node/node/out/Release/.deps//home/runner/work/node/node/node/out/Release/obj.target/libnode/src/node_url_pattern.o.d.raw   -c

I did try to build ada with -Wall -Wexta -Wno-unused-parameter but that resulted in other errors.

@anonrig
Copy link
Member

anonrig commented Feb 27, 2025

@richardlau appreciate the help: ada-url/ada#893

@anonrig anonrig closed this Feb 27, 2025
@anonrig anonrig deleted the actions/tools-update-ada branch February 27, 2025 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants