java_launcher: Fix non-portable ofstream usage #24703

BoleynSu · 2024-12-15T18:08:37Z

wofstream cannot be constructed from wstring according to the standard https://en.cppreference.com/w/cpp/io/basic_ofstream/basic_ofstream.

BoleynSu · 2024-12-15T18:41:19Z

src/tools/launcher/java_launcher.cc

  wstring jar_manifest_file_path =
      binary_base_path + rand_id + L".jar_manifest";
  blaze_util::AddUncPrefixMaybe(&jar_manifest_file_path);
+#if (__cplusplus >= 201703L)


This is vulerable to https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ but we do not care since under MSVC both versions will work anyway.

Bazel already requires C++ 17, so you may not need this conditional:

bazel/.bazelrc

Line 45 in 16f08cb

build:linux --cxxopt=-std=c++17

wofstream cannot be constructed from wstring according to the standard https://en.cppreference.com/w/cpp/io/basic_ofstream/basic_ofstream.

The guard is needed to pass the presubmit for MSVC

fmeum

Could you explain how this non-portable usage manifests in problems in the PR description?

fmeum · 2024-12-16T13:27:55Z

src/tools/launcher/java_launcher.cc

  wstring jar_manifest_file_path =
      binary_base_path + rand_id + L".jar_manifest";
  blaze_util::AddUncPrefixMaybe(&jar_manifest_file_path);
+#if (__cplusplus >= 201703L)


Bazel already requires C++ 17, so you may not need this conditional:

bazel/.bazelrc

Line 45 in 16f08cb

build:linux --cxxopt=-std=c++17

fmeum · 2024-12-16T16:55:42Z

@bazel-io fork 7.5.0

fmeum · 2024-12-16T16:55:51Z

@bazel-io fork 8.1.0

wofstream cannot be constructed from wstring according to the standard https://en.cppreference.com/w/cpp/io/basic_ofstream/basic_ofstream. Closes bazelbuild#24703. PiperOrigin-RevId: 707172911 Change-Id: I973f9560c5b20748e4635cd757f53fff9c8ef0c5

wofstream cannot be constructed from wstring according to the standard https://en.cppreference.com/w/cpp/io/basic_ofstream/basic_ofstream. Closes #24703. PiperOrigin-RevId: 707172911 Change-Id: I973f9560c5b20748e4635cd757f53fff9c8ef0c5 Commit 5b35276 Co-authored-by: Boleyn Su <[email protected]> Co-authored-by: Yun Peng <[email protected]>

BoleynSu · 2024-12-19T12:05:44Z

@fmeum should it be cherry picked to 8.0.1 instead of 8.1.0?

meteorcloudy · 2024-12-19T12:13:52Z

This is not a fix for a regression, so no need to cherry picked to 8.0.1

BoleynSu · 2024-12-19T12:25:32Z

Should the other PR be cherry picked to the same branch, i.e. 8.1.0, then? I was worried about the merge conflict.

wofstream cannot be constructed from wstring according to the standard https://en.cppreference.com/w/cpp/io/basic_ofstream/basic_ofstream. Closes #24703. PiperOrigin-RevId: 707172911 Change-Id: I973f9560c5b20748e4635cd757f53fff9c8ef0c5

wofstream cannot be constructed from wstring according to the standard https://en.cppreference.com/w/cpp/io/basic_ofstream/basic_ofstream. Closes bazelbuild#24703. PiperOrigin-RevId: 707172911 Change-Id: I973f9560c5b20748e4635cd757f53fff9c8ef0c5

wofstream cannot be constructed from wstring according to the standard https://en.cppreference.com/w/cpp/io/basic_ofstream/basic_ofstream. Closes #24703. PiperOrigin-RevId: 707172911 Change-Id: I973f9560c5b20748e4635cd757f53fff9c8ef0c5 Commit 5b35276 Co-authored-by: Boleyn Su <[email protected]>

iancha1992 · 2025-01-23T00:02:34Z

The changes in this PR have been included in Bazel 7.5.0 RC2. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.5.0rc2. Thanks!

wofstream cannot be constructed from wstring according to the standard https://en.cppreference.com/w/cpp/io/basic_ofstream/basic_ofstream. Closes bazelbuild#24703. PiperOrigin-RevId: 707172911 Change-Id: I973f9560c5b20748e4635cd757f53fff9c8ef0c5

meteorcloudy · 2025-02-18T14:58:56Z

Looks like this caused some breakages for the java_launcher with clang:

ERROR: C:/users/pcloudy/workdir/bazel/src/tools/launcher/BUILD:38:15: Compiling src/tools/launcher/java_launcher.cc failed: (Exit 1): clang-cl.exe failed: error executing CppCompile command (from target //src/tools/launcher:java_launcher) C:\Program Files\LLVM\bin\clang-cl.exe @bazel-out/x64_windows-fastbuild/bin/src/tools/launcher/_objs/java_launcher/java_launcher.obj.params
src/tools/launcher/java_launcher.cc(257,31): error: 'std::filesystem::path' is an incomplete type
  257 |   wofstream jar_manifest_file{std::filesystem::path(jar_manifest_file_path)};
      |                               ^
C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.39.33519\include\__msvc_filebuf.hpp(30,23): note: forward declaration of 'std::filesystem::path'
   30 |     _EXPORT_STD class path;
      |                       ^
In file included from src/tools/launcher/java_launcher.cc:15:
In file included from .\src/tools/launcher/java_launcher.h:21:
In file included from .\src/tools/launcher/launcher.h:22:
In file included from .\src/tools/launcher/util/data_parser.h:18:
C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.39.33519\include\fstream(213,31): error: member access into incomplete type 'const std::filesystem::path'
  213 |         : basic_ofstream(_Path.c_str(), _Mode, _Prot) {} // _Prot is an extension
      |                               ^
src/tools/launcher/java_launcher.cc(257,13): note: in instantiation of function template specialization 'std::basic_ofstream<wchar_t>::basic_ofstream<std::filesystem::path, 0>' requested here
  257 |   wofstream jar_manifest_file{std::filesystem::path(jar_manifest_file_path)};
      |             ^
C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.39.33519\include\__msvc_filebuf.hpp(30,23): note: forward declaration of 'std::filesystem::path'
   30 |     _EXPORT_STD class path;
      |                       ^
2 errors generated.
Target //src/tools/launcher:java_launcher failed to build                                                                                                                                                                                                                         
Use --verbose_failures to see the command lines of failed build steps.                                                                                                                                                                                                            
INFO: Elapsed time: 0.858s, Critical Path: 0.65s

Are we missing

#if (__cplusplus >= 201703L)
#include <filesystem>
#endif

This is blocking tensorflow/tensorflow#87159 since TF is built with clang on windows.

Addresses: #24703 (comment) PiperOrigin-RevId: 728571293 Change-Id: Ic1a292b6a6394e1c996cdaa1189e33431eb6f6d2

Addresses: bazelbuild#24703 (comment) PiperOrigin-RevId: 728571293 Change-Id: Ic1a292b6a6394e1c996cdaa1189e33431eb6f6d2

Addresses: #24703 (comment) PiperOrigin-RevId: 728571293 Change-Id: Ic1a292b6a6394e1c996cdaa1189e33431eb6f6d2 Commit 77137d4 Co-authored-by: Googler <[email protected]>

Addresses: bazelbuild#24703 (comment) PiperOrigin-RevId: 728571293 Change-Id: Ic1a292b6a6394e1c996cdaa1189e33431eb6f6d2

… clang-cl (#25467) Addresses: #24703 (comment) PiperOrigin-RevId: 728571293 Change-Id: Ic1a292b6a6394e1c996cdaa1189e33431eb6f6d2 Commit 77137d4 Co-authored-by: Googler <[email protected]>

github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Dec 15, 2024

BoleynSu commented Dec 15, 2024

View reviewed changes

java_launcher: Fix non-portable wofstream usage

fd691ed

BoleynSu force-pushed the patch-3 branch from 173f271 to fd691ed Compare December 15, 2024 18:47

sgowroji added the team-Rules-Java Issues for Java rules label Dec 16, 2024

fmeum reviewed Dec 16, 2024

View reviewed changes

fmeum requested a review from meteorcloudy December 16, 2024 16:01

fmeum approved these changes Dec 16, 2024

View reviewed changes

meteorcloudy approved these changes Dec 16, 2024

View reviewed changes

meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 16, 2024

This was referenced Dec 16, 2024

[7.5.0] java_launcher: Fix non-portable ofstream usage #24712

Closed

[8.1.0] java_launcher: Fix non-portable ofstream usage #24713

Closed

copybara-service bot closed this in 5b35276 Dec 17, 2024

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Dec 17, 2024

bazel-io mentioned this pull request Dec 17, 2024

[7.5.0] java_launcher: Fix non-portable ofstream usage #24725

Merged

bazel-io mentioned this pull request Jan 18, 2025

[8.1.0] java_launcher: Fix non-portable ofstream usage #24975

Merged

copybara-service bot pushed a commit that referenced this pull request Feb 19, 2025

Fix building binary launchers on windows with clang-cl

77137d4

Addresses: #24703 (comment) PiperOrigin-RevId: 728571293 Change-Id: Ic1a292b6a6394e1c996cdaa1189e33431eb6f6d2

bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Feb 20, 2025

Fix building binary launchers on windows with clang-cl

ee2c5e1

Addresses: bazelbuild#24703 (comment) PiperOrigin-RevId: 728571293 Change-Id: Ic1a292b6a6394e1c996cdaa1189e33431eb6f6d2

bazel-io mentioned this pull request Feb 20, 2025

[8.1.1] Fix building binary launchers on windows with clang-cl #25344

Merged

bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Feb 20, 2025

Fix building binary launchers on windows with clang-cl

4faa6bb

Addresses: bazelbuild#24703 (comment) PiperOrigin-RevId: 728571293 Change-Id: Ic1a292b6a6394e1c996cdaa1189e33431eb6f6d2

bazel-io mentioned this pull request Feb 20, 2025

[8.2.0] Fix building binary launchers on windows with clang-cl #25346

Merged

bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Mar 4, 2025

Fix building binary launchers on windows with clang-cl

ba998df

Addresses: bazelbuild#24703 (comment) PiperOrigin-RevId: 728571293 Change-Id: Ic1a292b6a6394e1c996cdaa1189e33431eb6f6d2

bazel-io mentioned this pull request Mar 4, 2025

[7.6.0] Fix building binary launchers on windows with clang-cl #25467

Merged

java_launcher: Fix non-portable ofstream usage #24703

java_launcher: Fix non-portable ofstream usage #24703

Uh oh!

Conversation

BoleynSu commented Dec 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BoleynSu Dec 15, 2024

Choose a reason for hiding this comment

Uh oh!

fmeum Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

BoleynSu Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

BoleynSu Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

fmeum left a comment

Choose a reason for hiding this comment

Uh oh!

fmeum Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

fmeum commented Dec 16, 2024

Uh oh!

fmeum commented Dec 16, 2024

Uh oh!

BoleynSu commented Dec 19, 2024

Uh oh!

meteorcloudy commented Dec 19, 2024

Uh oh!

BoleynSu commented Dec 19, 2024

Uh oh!

iancha1992 commented Jan 23, 2025

Uh oh!

meteorcloudy commented Feb 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BoleynSu commented Dec 15, 2024 •

edited

Loading