Bazel support for windows

[skeptic-monkey]

Using google/glog from a bazel project on windows doesn't work.
bazel/glog.bzl doesn't seem to be windows compatible (but does on linux and darwing). Is there a plan to add support for it ?

here is the error I get :

$> bazel build --compiler=clang-cl --cxxopt=/std:c++17 @com_google_glog//:glog
INFO: Writing tracer profile to 'C:/users/lucbe/_bazel_lucbe/ixjskkhv/command.profile.gz'
INFO: Analyzed target @com_google_glog//:glog (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
ERROR: C:/users/lucbe/_bazel_lucbe/ixjskkhv/external/com_google_glog/BUILD:5:1: C++ compilation of rule '@com_google_glog//:glog' failed (Exit 1)
In file included from external/com_google_glog/src/demangle.cc:38:
external/com_google_glog/src/utilities.h(80,11): fatal error: 'port.h' file not found
# include "port.h"
          ^~~~~~~~
1 error generated.
Target @com_google_glog//:glog failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 1.512s, Critical Path: 1.05s
INFO: 0 processes.
FAILED: Build did NOT complete successfully

[drigz]

I don't have plans to work on this, and I don't have much Windows dev experience. If someone wants to take it on, let me know and I'll happily provide guidance on the Bazel side of things.

The first step to resolving the port.h error would be to change bazel/glog.bzl to use select() to use the files in src/windows/ instead of the Linux/Darwin header files.

The PR that fixes the Windows build should also enable Windows CI by adding to .bazelci/presubmit.yml:

  windows:
    # Optional: use VS 2017 instead of 2015.
    environment:
      BAZEL_VC: "C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\BuildTools\\VC"
    build_targets:
    - "..."
    test_targets:
    - "..."

[mehrdadn]

Hi, for Windows support I think you need to do the following:

1. Add src/glog/log_severity.h to srcs

2. Add src/windows/config.h, src/windows/port.cc, src/windows/port.h as Windows-only srcs
   (via select({ "@bazel_tools//src/conditions:windows" = ... }))

3. Add src/windows/config.h and src/windows/port.h as Windows-only hdrs

4. Add the following Windows-only copts

    '-D_NO_CRT_STDIO_INLINE',  # snprintf conflict workaround
    '-I%s/src/windows' % __workspace_dir__,
   

5. Move the following into the non-Windows (//conditions:default) portion of the copts:

    # Allows src/base/mutex.h to include pthread.h.
    '-DHAVE_PTHREAD',
    # Allows src/logging.cc to determine the host name.
    '-DHAVE_SYS_UTSNAME_H',
    # For src/utilities.cc.
    '-DHAVE_SYS_SYSCALL_H',
    '-DHAVE_SYS_TIME_H',
    # Enable dumping stacktrace upon sigaction.
    '-DHAVE_SIGACTION',
   

6. Modify the gen_sh rule to set ac_cv_have_unistd_h to 0 on Windows.

Unfortunately I don't have an elegant solution for __workspace_dir__ or for gen_sh—do you know how to get the workspace directory and how to avoid stitching together the script in an ugly manner via select?

[drigz]

Modify the gen_sh rule to set ac_cv_have_unistd_h to 0 on Windows.

Do we need gen_sh on Windows? I had hoped the pregenerated headers in https://github.com/google/glog/tree/master/src/windows/glog would work. Using bash in Windows builds is not ideal as Bazel is trying to drop the dependency on bash/msys.

do you know how to get the workspace directory

If I understand correctly, __workspace_dir__ + '/src/windows' will be incorrect if glog is an external repository. Does the pattern from bazelbuild/bazel#4463 work? Something like:

    # The path to src/windows/ depends on whether glog is the main workspace or
    # an external repository.
    if native.repository_name() != '@':
        internal = 'external/%s/src/windows' % native.repository_name().lstrip('@')
    else:
        internal = 'src/windows'
    copts = ["-I" + internal],

how to avoid stitching together the script in an ugly manner via select?

One pattern I've seen for platform-dependent builds is to use variables to collect the common and platform-specific sources, eg https://github.com/nelhage/rules_boost/blob/13a7a40214bf62b6c5e44aa376db18c6315e67b2/BUILD.boost#L929

This avoids having lots of hard-to-read nesting since the cc_library rules, but you still need to select().

Hope this helps, thanks for looking in to the issue!

[skeptic-monkey]

I was also looking into it, and I have something working both on linux and windows.

But I faced an unexpected issue related to : bazelbuild/bazel#6337

IIUC, on linux we use the strip_include_prefix so external projects can include headers using glog/ prefix.
But on windows, strip_include_prefix doesn't seem to work correctly. which forces me to use the includes argument of cc_library, which isn't nice because it pollutes the compilation flags of project depending on glog.

I'll add a link to my current implementation soon.

[drigz]

FWIW, I don't think there's a big advantage of strip_include_prefix over includes. You could check by using -s, but I believe they will both affect the cflags of all dependent targets. If that's the only issue, please go ahead and send a PR!

(one difference: strip_include_prefix plays nice with include_prefix, which can be useful, but is rarely necessary)

[mehrdadn]

Modify the gen_sh rule to set ac_cv_have_unistd_h to 0 on Windows.

Do we need gen_sh on Windows? I had hoped the pregenerated headers in https://github.com/google/glog/tree/master/src/windows/glog would work.

I feel like it's possible, but when I try that I'm struggling to figure out how to resolve these errors that pop up:

ERROR: Couldn't build file external/com_github_google_glog/_objs/glog/raw_logging.obj: undeclared inclusion(s) in rule '@com_github_google_glog//:glog':
this rule is missing dependency declarations for the following files included by 'external/com_github_google_glog/src/raw_logging.cc':
  'bazel-out/x64_windows-fastbuild/bin/external/com_github_google_glog/_virtual_includes/glog/glog/logging.h'
  'bazel-out/x64_windows-fastbuild/bin/external/com_github_google_glog/_virtual_includes/glog/glog/vlog_is_on.h'
  'bazel-out/x64_windows-fastbuild/bin/external/com_github_google_glog/_virtual_includes/glog/glog/raw_logging.h'

Using bash in Windows builds is not ideal as Bazel is trying to drop the dependency on bash/msys.

Wow, did not know. I guess this means I'll have to port my build scripts to something else too. (Python?) Do you know why they haven't been able to drop the dependency so far?

do you know how to get the workspace directory

If I understand correctly, __workspace_dir__ + '/src/windows' will be incorrect if glog is an external repository. Does the pattern from bazelbuild/bazel#4463 work? Something like:

    # The path to src/windows/ depends on whether glog is the main workspace or an external repository.
    if native.repository_name() != '@':
        internal = 'external/%s/src/windows' % native.repository_name().lstrip('@')
    else:
        internal = 'src/windows'
    copts = ["-I" + internal],

Yes this works, thank you so much!

how to avoid stitching together the script in an ugly manner via select?

One pattern I've seen for platform-dependent builds is to use variables to collect the common and platform-specific sources, eg https://github.com/nelhage/rules_boost/blob/13a7a40214bf62b6c5e44aa376db18c6315e67b2/BUILD.boost#L929

This avoids having lots of hard-to-read nesting since the cc_library rules, but you still need to select().

Oh, but what I meant is that if you try this for the cmd script rather than for the file list, it turns quite ugly; you can't concatenate strings to select(...) (only lists), so you'll have to split up the Bash script into bits and then re-stitch them together for each platform. I don't know of a better way, it seems like a Blaze deficiency that select(...) doesn't work with strings.

[drigz]

Do we need gen_sh on Windows? I had hoped the pregenerated headers in https://github.com/google/glog/tree/master/src/windows/glog would work.

I feel like it's possible, but when I try that I'm struggling to figure out how to resolve these errors that pop up:

Hmm, sometimes missing dependency declarations is very straightforward and sometimes it's very ... whatever the opposite of straightforward is. Could you share a link to your branch?

Using bash in Windows builds is not ideal as Bazel is trying to drop the dependency on bash/msys.

Wow, did not know. I guess this means I'll have to port my build scripts to something else too. (Python?) Do you know why they haven't been able to drop the dependency so far?

You can always use it yourself if you want, but for a simple dependency like glog it would be nice to allow those who haven't installed msys to build it. There's a little bit of background here: https://blog.bazel.build/2017/10/16/windows-retrospect.html

how to avoid stitching together the script in an ugly manner via select?

One pattern I've seen for platform-dependent builds is to use variables to collect the common and platform-specific sources, eg https://github.com/nelhage/rules_boost/blob/13a7a40214bf62b6c5e44aa376db18c6315e67b2/BUILD.boost#L929
This avoids having lots of hard-to-read nesting since the cc_library rules, but you still need to select().

Oh, but what I meant is that if you try this for the cmd script rather than for the file list, it turns quite ugly; you can't concatenate strings to select(...) (only lists), so you'll have to split up the Bash script into bits and then re-stitch them together for each platform. I don't know of a better way, it seems like a Blaze deficiency that select(...) doesn't work with strings.

I'd expect the following to work, does it not? Alternatively we could stop inlining the script and have gen.windows.sh, gen.default.sh, but I'd hope that we can drop the bash dependency on Windows instead.

        cmd = r'''\
#!/bin/sh
cat > $@ <<"EOF"
sed -e 's/@ac_cv_cxx_using_operator@/1/g' \
    -e 's/@ac_cv_have_unistd_h@/{ac_cv_have_unistd_h}/g' \
    -e 's/@ac_cv_have_libgflags@/{ac_cv_have_libgflags}/g'
EOF
'''.format({
    "ac_cv_have_unistd_h": select({
        ":windows": 0,
        ":default": 1,
    }),
    "ac_cv_have_libgflags": int(with_gflags)),
})

[mehrdadn]

@drigz Sorry for the lack of a response by the way, I'd left the notification email unread in my inbox this whole time to get back to you with an example when I could get one ready... I just haven't managed to yet. Great to see progress was made regardless though. Thank you both!

[drigz]

@mehrdadn No problem! I've merged the PR now, maybe you could give it a try and see if it works for you.