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

skylark new_git_repository rules react different than native rule with build_file #2997

Closed
MarkusTeufelberger opened this issue May 11, 2017 · 5 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request

Comments

@MarkusTeufelberger
Copy link
Contributor

Please provide the following information. The more we know about your system and use case, the more easily and likely we can help.

Description of the problem / feature request / question:

When playing around with new_git_repository and after reading the hint that I should use the skylark ruleset from "@bazel_tools//tools/build_defs/repo:git.bzl" instead of the native "new_git_repository()" I ran into the issue that the skylark rules apparently seem to work differently after a bind command (see example).

While the native rule (only) accepts files in the root folder of the workspace as build_file, the skylark ruleset seems to look in a subfolder.

I would expect the skylark rule to be a drop-in replacement of the native rule, or at the very least that the expected build_file location is documented for that rule.

If possible, provide a minimal example to reproduce the problem:

foo@bar$ cat WORKSPACE 
# native rules use jgit, these skylark rules use native git:
#load("@bazel_tools//tools/build_defs/repo:git.bzl", "new_git_repository")

# align
new_git_repository(
    name = "boost_align_src",
    build_file = "boost_align.BUILD",
    remote = "https://github.com/boostorg/align",
    tag = "boost-1.64.0",
)

# //external:boost_align
bind(
    name = "boost_align",
    actual = "@boost_align_src//:boost_align",
)
foo@bar$ cat boost_align.BUILD 
cc_library(
    name = "boost_align_include",
    hdrs = [
        "include/boost/align/align.hpp",
        "include/boost/align/align_down.hpp",
        "include/boost/align/align_up.hpp",
        "include/boost/align/aligned_alloc.hpp",
        "include/boost/align/aligned_allocator.hpp",
        "include/boost/align/aligned_allocator_adaptor.hpp",
        "include/boost/align/aligned_allocator_adaptor_forward.hpp",
        "include/boost/align/aligned_allocator_forward.hpp",
        "include/boost/align/aligned_delete.hpp",
        "include/boost/align/aligned_delete_forward.hpp",
        "include/boost/align/alignment_of.hpp",
        "include/boost/align/alignment_of_forward.hpp",
        "include/boost/align/assume_aligned.hpp",
        "include/boost/align/detail/addressof.hpp",
        "include/boost/align/detail/align.hpp",
        "include/boost/align/detail/align_cxx11.hpp",
        "include/boost/align/detail/align_down.hpp",
        "include/boost/align/detail/align_up.hpp",
        "include/boost/align/detail/aligned_alloc.hpp",
        "include/boost/align/detail/aligned_alloc_android.hpp",
        "include/boost/align/detail/aligned_alloc_macos.hpp",
        "include/boost/align/detail/aligned_alloc_msvc.hpp",
        "include/boost/align/detail/aligned_alloc_posix.hpp",
        "include/boost/align/detail/aligned_alloc_sunos.hpp",
        "include/boost/align/detail/alignment_of.hpp",
        "include/boost/align/detail/alignment_of_clang.hpp",
        "include/boost/align/detail/alignment_of_codegear.hpp",
        "include/boost/align/detail/alignment_of_cxx11.hpp",
        "include/boost/align/detail/alignment_of_gcc.hpp",
        "include/boost/align/detail/alignment_of_msvc.hpp",
        "include/boost/align/detail/assume_aligned.hpp",
        "include/boost/align/detail/assume_aligned_clang.hpp",
        "include/boost/align/detail/assume_aligned_gcc.hpp",
        "include/boost/align/detail/assume_aligned_intel.hpp",
        "include/boost/align/detail/assume_aligned_msvc.hpp",
        "include/boost/align/detail/element_type.hpp",
        "include/boost/align/detail/integral_constant.hpp",
        "include/boost/align/detail/is_aligned.hpp",
        "include/boost/align/detail/is_alignment.hpp",
        "include/boost/align/detail/is_alignment_constant.hpp",
        "include/boost/align/detail/max_align.hpp",
        "include/boost/align/detail/max_objects.hpp",
        "include/boost/align/detail/max_size.hpp",
        "include/boost/align/detail/min_size.hpp",
        "include/boost/align/is_aligned.hpp",
    ],
    strip_include_prefix = "include/",
    visibility = ["//visibility:private"],
)

cc_library(
    name = "boost_align",
    hdrs = ["include/boost/align.hpp"],
    strip_include_prefix = "include/",
    visibility = ["//visibility:public"],
    deps = [":boost_align_include"],
)
foo@bar$ cat BUILD 
cc_binary(
    name = "dummy",
    srcs = ["helloworld.cc"],
    deps = [
        "//external:boost_align",
    ],
)
foo@bar$ cat helloworld.cc 
#include <stdio.h>
// test includes:
#include <boost/align/align.hpp>

int main()
{
    printf("Hello World!\n");
foo@bar$ bazel build //...
......
WARNING: Sandboxed execution is not supported on your system and thus hermeticity of actions cannot be guaranteed. See http://bazel.build/docs/bazel-user-manual.html#sandboxing for more information. You can turn off this warning via --ignore_unsupported_sandboxing.
INFO: Found 1 target...
Target //:dummy up-to-date:
  bazel-bin/dummy
INFO: Elapsed time: 2.497s, Critical Path: 0.20s

now with the skylark rule uncommented:

foo@bar$ bazel build //...
WARNING: Sandboxed execution is not supported on your system and thus hermeticity of actions cannot be guaranteed. See http://bazel.build/docs/bazel-user-manual.html#sandboxing for more information. You can turn off this warning via --ignore_unsupported_sandboxing.
ERROR: /home/xxx/WORKSPACE:13:1: no such package '@boost_align_src//': Not a file: /home/xxx/external/boost_align.BUILD and referenced by '//external:boost_align'.
ERROR: Analysis of target '//:dummy' failed; build aborted.
INFO: Elapsed time: 4.701s

Apparently the "bind()" rule later on seems strong enough to cause skylark to expect the BUILD file to be in a sub-folder named "external", which doesn't exist.

(The choice of the boost align library is unrelated to this, it is just something that I want to write some rules for)

Environment info

  • Operating System:
    Manjaro Linux

  • Bazel version (output of bazel info release):
    release 0.4.5- (@non-git)

  • If bazel info release returns "development version" or "(@non-git)", please tell us what source tree you compiled Bazel from; git commit hash is appreciated (git rev-parse HEAD):

https://www.archlinux.org/packages/community/x86_64/bazel/

Have you found anything relevant by searching the web?

I wish...

@kchodorow
Copy link
Contributor

kchodorow commented May 12, 2017

I think it should be consistent between both native & skylark rules if you do:

build_file = "//:boost_align.BUILD",

...but you are correct, these should be consistent.

Edited to add: but I'd recommend not using bind() anyway, so if that's an option, prefer that.

@MarkusTeufelberger
Copy link
Contributor Author

I'd recommend not using bind() anyway

Why? In the end it seemed to me that the //external package is the most logical place for external dependencies to reside in.

Anyways, yes, specifying the full path instead of just the file name seems to work.

@kchodorow
Copy link
Contributor

I implemented it, but in retrospect I think it was a mistake. Labels in Bazel are nice in that you know where to find everything: //x/y is in the directory project/x/y. @foo//x/y is under /foo/x/y. //external breaks this: you have no idea where something defined by external was defined. This will be especially bad once #1943 is implemented and you have multiple WORKSPACE files contributing definitions.

Feel free to chime in on #1952, which is a long discussion about possibly removing bind.

@MarkusTeufelberger
Copy link
Contributor Author

I assumed that bind() is a nice way to keep external dependencies abstract (e.g. if everyone depends on //external:foo it is easier to switch foo 1.0.0 for 1.0.1 without having to rewrite every dependency that uses foo).

Beyond that I don't have many arguments for/against bind(), I agree that it looks kinda weird with that "virtual" //external namespace, though I also find it nice to have a way to explicitly state that something is loaded from the outside. This is more me trying out to get some builds going rather than putting my foot down in a "you'll take bind() from my cold dead hands" kinda way. It would be nice though to get a few best practices going, but that's anyways also in the interest of the BUILD file repo task force. :-)

@kchodorow
Copy link
Contributor

kchodorow commented May 31, 2017

:) We actually do have a start of a "Best practices" doc: https://bazel.build/versions/master/docs/best-practices.html#repository-rules.

Generally we recommend using names like @foo (instead of @foo-3.1) to avoid version rewrites.

@dslomov dslomov added P2 We'll consider working on this in future. (Assignee optional) external-repos-triaged P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P2 We'll consider working on this in future. (Assignee optional) labels Jan 10, 2018
@dslomov dslomov closed this as completed Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request
Projects
None yet
Development

No branches or pull requests

3 participants