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

Rebuild docker images #17568

Merged
merged 3 commits into from
Jun 10, 2024
Merged

Rebuild docker images #17568

merged 3 commits into from
Jun 10, 2024

Conversation

jpienaar
Copy link
Member

@jpienaar jpienaar commented Jun 4, 2024

No description provided.

build:remote_cache_bazel_ci --host_platform_remote_properties_override='properties:{name:"cache-silo-key" value:"gcr.io/iree-oss/base-bleeding-edge@sha256:c5f28883e6c570c20128fb37d7af3a00a25df3ce4e2b3a24c3a8dcd183182a27"}'
build:remote_cache_bazel_ci --host_platform_remote_properties_override='properties:{name:"cache-silo-key" value:"gcr.io/iree-oss/base-bleeding-edge@sha256:cf2e78194e64fd0166f4141317366261d7a62432b72e9a324cb8c2ff4e1a515a"}'
Copy link
Member

Choose a reason for hiding this comment

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

Bazel build errors 🤔 https://github.com/iree-org/iree/actions/runs/9370945221/job/25799102320?pr=17568#step:4:207

ERROR: /work/runtime/src/iree/hal/drivers/vulkan/BUILD.bazel:90:24: Compiling runtime/src/iree/hal/drivers/vulkan/dynamic_symbols.cc failed: (Exit 1): clang failed: error executing command (from target //runtime/src/iree/hal/drivers/vulkan:dynamic_symbols) /usr/lib/llvm-19/bin/clang -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics -fno-omit-frame-pointer -g0 ... (remaining 95 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
runtime/src/iree/hal/drivers/vulkan/dynamic_symbols.cc:148:3: error: passing no argument for the '...' parameter of a variadic macro is a C++20 extension [-Werror,-Wc++20-extensions]
  148 |   IREE_RETURN_IF_ERROR(ResolveFunctions(syms.get(), get_proc_addr));
      |   ^
runtime/src/iree/base/status.h:370:3: note: expanded from macro 'IREE_RETURN_IF_ERROR'
  370 |   IREE_STATUS_IMPL_RETURN_IF_API_ERROR_(                \

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, could it be an actual C++20 feature and just never flagged before? That's curious. Haven't checked yet.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah idk, we should still be compiling in c++17 mode:

###############################################################################
# Options for "generic_clang" builds: these options should generally apply to
# either clang or gcc and are curated based on need.
###############################################################################
# C++17 standard version is required.
build:generic_clang --cxxopt=-std=c++17 --host_cxxopt=-std=c++17

--config=generic_clang is on the command: https://github.com/iree-org/iree/actions/runs/9370945221/job/25799102320?pr=17568#step:4:174

Copy link
Member Author

Choose a reason for hiding this comment

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

I just don't see this error ... meaning, an argument is passed in . I see #16946 so perhaps the solution is just to disable that same check for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

A trick like https://stackoverflow.com/questions/11761703/overloading-macro-on-number-of-arguments could be used perhaps. Or we just disable the check given things are working and the world moves to C++20 "soon" :)

Comment on lines +93 to +94
# This is currently flagging the status macros (https://github.com/iree-org/iree/issues/16946)
build:generic_clang --copt=-Wno-c++20-extensions --host_copt=-Wno-c++20-extensions
Copy link
Member

Choose a reason for hiding this comment

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

also remove from lower in the file

# Explicitly enable some additional warnings.
# Some of these aren't on by default, or under -Wall, or are subsets of warnings
# turned off above.
build:generic_clang --copt=-Wc++20-extensions

@jpienaar
Copy link
Member Author

jpienaar commented Jun 6, 2024

This has tests failing on Windows now.

@ScottTodd
Copy link
Member

This has tests failing on Windows now.

You can ignore those. https://github.com/actions/runner-images rolled out a bad image and isn't reverting for some reason

@ScottTodd
Copy link
Member

We don't use Docker or Bazel on Windows, so no way this could have affected those builds, working around the failures in #17595

@jpienaar
Copy link
Member Author

Sorry missed response, SG to submit then.

@jpienaar jpienaar merged commit cda9fd1 into main Jun 10, 2024
53 of 55 checks passed
@jpienaar jpienaar deleted the bump_docker branch June 10, 2024 17:47
@ScottTodd
Copy link
Member

FYI, this appears to have regressed some VMVX benchmarks: https://perf.iree.dev/serie?IREE?007d0bd606ea265d9639e4ec12d65e427dfdbc2c564e72cdd5e3c78a6fa198a6 (5800ms -> 7900ms)

I'm assuming that came from this commit and not some other environment change (e.g. to the runner machines). Maybe due to updating clang or some other toolchain? VMVX may be more sensitive to host compiler optimizations.

LLITCHEV pushed a commit to LLITCHEV/iree that referenced this pull request Jul 30, 2024
Disables -Wc++20-extensions for now.

Signed-off-by: Lubo Litchev <lubol@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants