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 ↗(On Diff #387853)

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 ↗(On Diff #387853)

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 ↗(On Diff #387853)

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 ↗(On Diff #387853)

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 ↗(On Diff #387853)

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 ↗(On Diff #387853)

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/
include/
llvm/
Config/
2 lines

Diff 391506

utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h

Show All 31 Lines
/* Define to 1 to enable crash overrides, and to 0 otherwise. */ /* Define to 1 to enable crash overrides, and to 0 otherwise. */
#define ENABLE_CRASH_OVERRIDES 1 #define ENABLE_CRASH_OVERRIDES 1
/* Define to 1 to enable crash memory dumps, and to 0 otherwise. */ /* Define to 1 to enable crash memory dumps, and to 0 otherwise. */
#define LLVM_ENABLE_CRASH_DUMPS 0 #define LLVM_ENABLE_CRASH_DUMPS 0
/* Define to 1 to prefer forward slashes on Windows, and to 0 prefer /* Define to 1 to prefer forward slashes on Windows, and to 0 prefer
backslashes. */ backslashes. */
/* #undef LLVM_WINDOWS_PREFER_FORWARD_SLASH */ #define LLVM_WINDOWS_PREFER_FORWARD_SLASH 0
/* Define to 1 if you have the `backtrace' function. */ /* Define to 1 if you have the `backtrace' function. */
/* HAVE_BACKTRACE defined in Bazel */ /* HAVE_BACKTRACE defined in Bazel */
/* BACKTRACE_HEADER defined in Bazel */ /* BACKTRACE_HEADER defined in Bazel */
/* Define to 1 if you have the <CrashReporterClient.h> header file. */ /* Define to 1 if you have the <CrashReporterClient.h> header file. */
/* HAVE_CRASHREPORTERCLIENT_H defined in Bazel */ /* HAVE_CRASHREPORTERCLIENT_H defined in Bazel */
▲ Show 20 Lines • Show All 340 Lines • Show Last 20 Lines