This is an archive of the discontinued LLVM Phabricator instance.

Differential D114065

[Bazel] Set the right default for LLVM_WINDOWS_PREFER_FORWARD_SLASH on Windows
ClosedPublic

Authored by GMNGeoffrey on Nov 17 2021, 12:18 AM.

Details

Reviewers
tpopp
mstorsjo
Commits
rGdc5e1d06b917: [Bazel] Set the right default for LLVM_WINDOWS_PREFER_FORWARD_SLASH on Windows
Summary

This cmake configure option was added in
df0ba47c36f6bd0865e3286853b76d37e037c2d7, and was ported to
Bazel in 7d323dc7738e3152c4bd54a23ac27554bfbbf583.

However, the setting chosen in Bazel seems accidental, not necessarily
intentional.

LLVM_WINDOWS_PREFER_FORWARD_SLASH has no effect on Unix, and on
Windows, setting it to 0 is the default, which gets the same behaviour
as before. Setting it to 1 enables new experimental behaviours
(which is enabled by default on MinGW targets only).

As I don't see any explicit intent to opt in to the new experimental
behaviour, I believe the current configuration in bazel was a
mistake.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Nov 17 2021, 12:18 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 12:18 AM
Harbormaster completed remote builds in B134679: Diff 387853.Nov 17 2021, 12:22 AM
GMNGeoffrey added a comment.Nov 17 2021, 1:42 PM

Thanks for catching this! This functionally does the right thing, but I think for consistency it would be better to spell it slightly differently

utils/bazel/llvm-project-overlay/llvm/config.bzl
39

So this is a noop? Maybe we should just leave it off then entirely then? There's also some housekeeping stuff that didn't get done in the commit you referenced. We usually keep track of the options in https://github.com/llvm/llvm-project/blob/main/utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h as well. They're either set directly there or it mentions that they're defined in Bazel. This whole system needs to be reworked, but better to stay consistent for now. If you wouldn't mind removing mentions of LLVM_WINDOWS_PREFER_FORWARD_SLASH in this file and adding it in the approrpiatel place in the config.h overlay file, that would be great (otherwise, I'll get to it at some point)

mstorsjo added inline comments.Nov 17 2021, 2:32 PM
utils/bazel/llvm-project-overlay/llvm/config.bzl
39

Well if you could take care of it, I'd appreciate it - I don't use Bazel myself, I just spotted that this new option was ported from cmake to Bazel in a possibly unintended way - I've been trying to reach out in other ways before, without reaction :-)

I.e., feel free to commandeer this revision or handle it otherwise - but good to have gotten your attention!

GMNGeoffrey added inline comments.Nov 17 2021, 3:05 PM
utils/bazel/llvm-project-overlay/llvm/config.bzl
39

Ah well most appreciated then, thank you! I will take care of it :-) What were the other methods you tried for communication? I don't think I saw any messages

mstorsjo added inline comments.Nov 17 2021, 3:15 PM
utils/bazel/llvm-project-overlay/llvm/config.bzl
39

I commented on the commit on GitHub (https://github.com/llvm/llvm-project/commit/7d323dc7738e3152c4bd54a23ac27554bfbbf583) and tried mailing Tres directly. Normally I would have replied to the mail on llvm-commits if nothing else, but commits that only touch the bazel directory didn’t seem to go there.

GMNGeoffrey added inline comments.Nov 17 2021, 3:36 PM
utils/bazel/llvm-project-overlay/llvm/config.bzl
39

Ah yeah. I'm not sure anyone follows GitHub commit comments. I thought Bazel phab reviews go to llvm-commits now. At least this one did. But it looks like direct commits don't :-/ Anyway, thanks for raising this :-)

tpopp added inline comments.Nov 22 2021, 1:56 AM
utils/bazel/llvm-project-overlay/llvm/config.bzl
39

I commented on the commit on GitHub (https://github.com/llvm/llvm-project/commit/7d323dc7738e3152c4bd54a23ac27554bfbbf583) and tried mailing Tres directly. Normally I would have replied to the mail on llvm-commits if nothing else, but commits that only touch the bazel directory didn’t seem to go there.

I'm sorry about this. I'll take a look at my email filters.

GMNGeoffrey commandeered this revision.Dec 2 2021, 5:27 PM
GMNGeoffrey edited reviewers, added: mstorsjo; removed: GMNGeoffrey.
GMNGeoffrey updated this revision to Diff 391505.Dec 2 2021, 5:29 PM

Avoid configuring in Bazel at all

Harbormaster completed remote builds in B137267: Diff 391505.Dec 2 2021, 5:32 PM
GMNGeoffrey updated this revision to Diff 391506.Dec 2 2021, 5:33 PM

Explicitly set to 0. I missed this was a cmakedefine01, not a cmakedefine

Harbormaster completed remote builds in B137269: Diff 391506.Dec 2 2021, 5:34 PM
GMNGeoffrey updated this revision to Diff 391507.Dec 2 2021, 5:36 PM

Diff against the right base

Harbormaster completed remote builds in B137270: Diff 391507.Dec 2 2021, 5:43 PM
This revision was not accepted when it landed; it landed in state Needs Review.Dec 2 2021, 5:45 PM
Closed by commit rGdc5e1d06b917: [Bazel] Set the right default for LLVM_WINDOWS_PREFER_FORWARD_SLASH on Windows (authored by GMNGeoffrey). · Explain Why
This revision was automatically updated to reflect the committed changes.
GMNGeoffrey added a commit: rGdc5e1d06b917: [Bazel] Set the right default for LLVM_WINDOWS_PREFER_FORWARD_SLASH on Windows.

Revision Contents

PathSize
utils/
bazel/
llvm-project-overlay/
llvm/
2 lines

Diff 387853

utils/bazel/llvm-project-overlay/llvm/config.bzl

Show All 30 Linesposix_defines = [
"HAVE_PTHREAD_GETSPECIFIC=1", "HAVE_PTHREAD_GETSPECIFIC=1",
"HAVE_PTHREAD_H=1", "HAVE_PTHREAD_H=1",
"HAVE_PTHREAD_SETNAME_NP=1", "HAVE_PTHREAD_SETNAME_NP=1",
"HAVE_REGISTER_FRAME=1", "HAVE_REGISTER_FRAME=1",
"HAVE_SETENV_R=1", "HAVE_SETENV_R=1",
"HAVE_STRERROR_R=1", "HAVE_STRERROR_R=1",
"HAVE_SYSEXITS_H=1", "HAVE_SYSEXITS_H=1",
"HAVE_UNISTD_H=1", "HAVE_UNISTD_H=1",
"LLVM_WINDOWS_PREFER_FORWARD_SLASH=0", "LLVM_WINDOWS_PREFER_FORWARD_SLASH=0",
GMNGeoffreyAuthorUnsubmitted
Not Done
ReplyInline Actions

So this is a noop? Maybe we should just leave it off then entirely then? There's also some housekeeping stuff that didn't get done in the commit you referenced. We usually keep track of the options in https://github.com/llvm/llvm-project/blob/main/utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h as well. They're either set directly there or it mentions that they're defined in Bazel. This whole system needs to be reworked, but better to stay consistent for now. If you wouldn't mind removing mentions of LLVM_WINDOWS_PREFER_FORWARD_SLASH in this file and adding it in the approrpiatel place in the config.h overlay file, that would be great (otherwise, I'll get to it at some point)

GMNGeoffrey: So this is a noop? Maybe we should just leave it off then entirely then? There's also some…
mstorsjoUnsubmitted
Done
ReplyInline Actions

Well if you could take care of it, I'd appreciate it - I don't use Bazel myself, I just spotted that this new option was ported from cmake to Bazel in a possibly unintended way - I've been trying to reach out in other ways before, without reaction :-)

I.e., feel free to commandeer this revision or handle it otherwise - but good to have gotten your attention!

mstorsjo: Well if you could take care of it, I'd appreciate it - I don't use Bazel myself, I just spotted…
GMNGeoffreyAuthorUnsubmitted
Not Done
ReplyInline Actions

Ah well most appreciated then, thank you! I will take care of it :-) What were the other methods you tried for communication? I don't think I saw any messages

GMNGeoffrey: Ah well most appreciated then, thank you! I will take care of it :-) What were the other…
mstorsjoUnsubmitted
Done
ReplyInline Actions

I commented on the commit on GitHub (https://github.com/llvm/llvm-project/commit/7d323dc7738e3152c4bd54a23ac27554bfbbf583) and tried mailing Tres directly. Normally I would have replied to the mail on llvm-commits if nothing else, but commits that only touch the bazel directory didn’t seem to go there.

mstorsjo: I commented on the commit on GitHub (https://github.com/llvm/llvm…
GMNGeoffreyAuthorUnsubmitted
Not Done
ReplyInline Actions

Ah yeah. I'm not sure anyone follows GitHub commit comments. I thought Bazel phab reviews go to llvm-commits now. At least this one did. But it looks like direct commits don't :-/ Anyway, thanks for raising this :-)

GMNGeoffrey: Ah yeah. I'm not sure anyone follows GitHub commit comments. I thought Bazel phab reviews go to…
tpoppUnsubmitted
Not Done
ReplyInline Actions

I commented on the commit on GitHub (https://github.com/llvm/llvm-project/commit/7d323dc7738e3152c4bd54a23ac27554bfbbf583) and tried mailing Tres directly. Normally I would have replied to the mail on llvm-commits if nothing else, but commits that only touch the bazel directory didn’t seem to go there.

I'm sorry about this. I'll take a look at my email filters.

tpopp: > I commented on the commit on GitHub (https://github.com/llvm/llvm…
] ]
linux_defines = posix_defines + [ linux_defines = posix_defines + [
"_GNU_SOURCE", "_GNU_SOURCE",
"HAVE_LINK_H=1", "HAVE_LINK_H=1",
"HAVE_LSEEK64=1", "HAVE_LSEEK64=1",
"HAVE_MALLINFO=1", "HAVE_MALLINFO=1",
"HAVE_POSIX_FALLOCATE=1", "HAVE_POSIX_FALLOCATE=1",
Show All 18 Lineswin32_defines = [
"_SCL_SECURE_NO_DEPRECATE", "_SCL_SECURE_NO_DEPRECATE",
"_SCL_SECURE_NO_WARNINGS", "_SCL_SECURE_NO_WARNINGS",
"UNICODE", "UNICODE",
"_UNICODE", "_UNICODE",
# LLVM features # LLVM features
r'LTDL_SHLIB_EXT=\".dll\"', r'LTDL_SHLIB_EXT=\".dll\"',
r'LLVM_PLUGIN_EXT=\".dll\"', r'LLVM_PLUGIN_EXT=\".dll\"',
"LLVM_WINDOWS_PREFER_FORWARD_SLASH=1", "LLVM_WINDOWS_PREFER_FORWARD_SLASH=0",
] ]
# TODO: We should switch to platforms-based config settings to make this easier # TODO: We should switch to platforms-based config settings to make this easier
# to express. # to express.
os_defines = select({ os_defines = select({
"@bazel_tools//src/conditions:windows": win32_defines, "@bazel_tools//src/conditions:windows": win32_defines,
"@bazel_tools//src/conditions:darwin": macos_defines, "@bazel_tools//src/conditions:darwin": macos_defines,
"@bazel_tools//src/conditions:freebsd": posix_defines, "@bazel_tools//src/conditions:freebsd": posix_defines,
Show All 20 Lines