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

genrule "outs" is not configurable via select() #281

Closed
kayasoze opened this issue Jul 3, 2015 · 14 comments
Closed

genrule "outs" is not configurable via select() #281

kayasoze opened this issue Jul 3, 2015 · 14 comments
Assignees

Comments

@kayasoze
Copy link

kayasoze commented Jul 3, 2015

This makes it a bit difficult to use a single genrule which produces different files on different platforms (i.e. foo.so, foo.dylib). It's not ideal to use a genrule to create libraries, but is very convenient for wrapping libraries built by autotools or CMake.

@laszlocsomor
Copy link
Contributor

Greg, could you take a look please?

@kayasoze kayasoze changed the title genrule "outs" is not configurable genrule "outs" is not configurable via select() Jul 7, 2015
@gregestren
Copy link
Contributor

Rule outputs are "special" in that they have to be defined unambiguously based purely on the rule definition. So we can't have the same genrule produce different outputs depending on what config parameters are passed at the command line (which is what fitting them into a select would mean). We might be able to lift this restriction in the future, but I don't think it would be quick or easy.

Instead, can you define platform-specific genrules then have whoever depends on them use a select to choose the appropriate dependency?

@kayasoze
Copy link
Author

That's not an optimal solution, but it could do in a pinch.

It looks like static outs would also complicate building Boost within Bazel, as Boost contains around 10,000 header files, all of which would need to be in outs in order to be used by dependencies.

@ulfjack
Copy link
Contributor

ulfjack commented Sep 23, 2015

Pardon my ignorance. Are you saying the boost header files are all automatically generated?

@kayasoze
Copy link
Author

Sorry, let me be clearer. If I were to wrap Boost's build system from Bazel, I imagine it would look something like this:

genrule(
  name = "build-boost",
  srcs = [":boost-srcs"],
  tools = [":bootstrap-boost"],
  outs = [
    # Should 10,000 headers go here?  
    ...
  ],
  cmd = "export PATH=$$PATH:/bin && export WORKSPACE=$$PWD && " +
        "cd %s && " % PACKAGE_NAME +
        "$$WORKSPACE/$(location bootstrap-boost) " +
        "--build-dir=$$WORKSPACE/$(GENDIR)/%s/build " % PACKAGE_NAME +
        "--prefix=$$WORKSPACE/$(GENDIR)/%s/install " % PACKAGE_NAME +
        "-j8 install"
)

cc_library(
  name = "headers",
  srcs = [":build-boost"],
  includes = ["install/include"],
  hdrs_check = "loose",  # Necessary without including all in "outs" above?
  linkstatic = 1,
  visibility = ["//visibility:public"],
)

The genrule above produces roughly 10,000 header files. This would imply to me that I must use hdrs_check = "loose" to wrap these headers, as otherwise they would all need to be enumerated in outs.

In practice, however, it turns out that none of the generated headers produced to $$WORKSPACE/$(GENDIR)/%s/install need to be declared in outs; hdrs_check = "strict" in the wrapper library makes no difference. I would have expected that hdrs_check = "loose" would be necessary in this case.

@kayasoze
Copy link
Author

kayasoze commented Oct 8, 2015

As it happens, hdrs_check = "loose" is not sufficient to build Boost. While the includes attribute implicitly covers generated files in $(GENDIR), adding a glob() directly to outs does not, because a path generated by glob in outs will never check $(GENDIR) (possibly because it is an absolute path internally).

This strategy will work in practice:

BOOST_SOURCE_HEADERS = glob(["boost/**/*.hpp", "boost/**/*.ipp", "boost/**/*.h"])
BOOST_INSTALLED_HEADERS = ["install/include/%s" % header for header in BOOST_SOURCE_HEADERS]

genrule(
  name = "build-boost",
  srcs = [":boost-srcs"],
  tools = [":bootstrap-boost"],
  outs = [ ...
  ] + BOOST_INSTALLED_HEADERS,

... however, it is only safe if the glob() and list-comprehension are lazily evaluated when the genrule is evaluated. I suspect this is the case, but I am not certain. Otherwise, incremental rebuilds will break. Can you confirm?

@ulfjack
Copy link
Contributor

ulfjack commented Oct 19, 2015

Are the header files modified as part of the bootstrap? Are they copied into the output tree?

What isn't safe?

@kayasoze
Copy link
Author

They are not modified as part of the bootstrap, as near as I can tell. They are indeed copied into the output tree.

My concern is the context in which the following expression is evaluated:

BOOST_SOURCE_HEADERS = glob(["boost/**/*.hpp", "boost/**/*.ipp", "boost/**/*.h"])

It's created outside of a rule, so I imagine there is a small chance that it is evaluated outside of (before) a rule as well. If so, I could create a new .hpp file in the filesystem (in boost/) and the incremental build would not pick it up. I expect it is evaluated on every up-to-date check on every rule which references it, but would like to confirm.

If glob() is evaluated on every up-to-date check of the genrule(), where it is used in outs, it opens up the possibility of using it to dynamically adjust outs. However, since outs cannot contain select() I expect the tolerance of glob() in outs is a bug.

@ulfjack
Copy link
Contributor

ulfjack commented Oct 19, 2015

Globs are evaluated over the source tree, and Bazel ensures that new or removed files get picked up on incremental builds. We don't re-evaluate the glob unless it has changed. We either use inotify (if enabled, except on MacOS) or we do a bulk stat+readdir operation over the source tree.

@ulfjack
Copy link
Contributor

ulfjack commented Oct 19, 2015

I guess that sounds a little bit weird, so let me try to clarify:

We have a Skyframe node (in memory) corresponding to the glob evaluation, and that depends on nodes corresponding to readdir and stat calls. On incremental builds, we go through all readdir and stat nodes, and rerun the corresponding operations (if inotify is enabled, we get the list of such nodes by listening to inotify events). If they return the same result, we never look at the node corresponding to the glob, and (transitively) don't redo any of the work that depends on the glob.

@kayasoze
Copy link
Author

It sounds like it's safe to use, then. Great work! It sounds like outs can indeed change across rebuilds.

@4ZM
Copy link

4ZM commented Jul 18, 2018

This is still an issue for me. I want to create a .tar.gz with a filename that contains fragments of build info. Please consider re-opening.

Ex. someoutput-dbg-k8.tar.gz or someoutput-opt-arm.tar.gz .. It would be nice to be able to select the suffix strings and use in outputs of the genrule that creates the tar archive. Another option would be to expand make variables in the outs so that I can do $(TARGET_CPU) etc and include in filename.

@dmitrychepel
Copy link

for me it is important too.
There are no ways to configure name of libs depending of platform.
only genrule could help, but it is not configurable.

@ulfjack
Copy link
Contributor

ulfjack commented Apr 4, 2019

I don't think that the claim "There are no ways to configure name of libs depending of platform" is correct.

genrule.outs is not configurable via select by design. The string passed to outs becomes a target in the package, i.e., a named thing that can be referenced from other BUILD files. If the value depended on the configuration (via select), then it would no longer be possible to reference it from other BUILD files.

Example:

foo/BUILD:
genrule(name = "foo", outs = ["a", "b"])

You can reference the rule //foo:foo, and you can reference individual outputs //foo:a and //foo:b. If the file looked like this:

foo/BUILD:
genrule(name = "foo", outs = select({"x" -> ["a", "b"]}))

Then you could no longer reliably reference //foo:a or //foo:b as their existence depends on the outcome of the select condition. Note that Bazel also guarantees that there are no conflicting targets in the same package, i.e., this is an error:

foo/BUILD:
genrule(name = "foo", outs = ["a", "b"])
genrule(name = "fooonlyb", outs = ["b"])

However, it is possible to write something like a genrule that does not add outs to the package, and this is possible today with a simple Starlark rule. This is an incomplete and untested example of how that would look like:

def _impl(ctx):
  x = ctx.actions.declare_file(ctx.attr.output_name)
  ctx.actions.run(...)
  return struct(files = [x])

my_rule = rule(
    implementation = _impl,
    attrs = {
        "output_name": attr.string(),
    },
)

Another alternative is to declare different names using multiple rules, but I take it that's not what you want to do.

foo/BUILD:
genrule(name = "foo_windows", outs = ["foo.dll"])
genrule(name = "foo_max", outs = ["foo.dylib"])
genrule(name = "foo_linux", outs = ["foo.so"])

NopeNopeGuy pushed a commit to NopeNopeGuy/kernel_build that referenced this issue Jun 1, 2024
To support select() properly, the argument "outs" in
kernel_build macro cannot be parsed, and it needs to be
passed to _kernel_build directly. Hence, outs (and
for consistency, module_outs and implicit_outs) of _kernel_build
are changed to a string_list.

Then inside _kernel_build_impl, declare_file() of the proper
output file. Because _kernel_build_impl already explicitly
return DefaultInfo(), this is okay.

To continue to support e.g. "kernel_aarch64/vmlinux", filegroups
with output groups are created to point to the correct file:

  kernel_build(name = "kernel_aarch64", out = ["vmlinux"])

becomes

  _kernel_build(name = "kernel_aarch64", ...)
  filegroup(name = "kernel_aarch64/vmlinux",
            srcs = ["kernel_aarch64"],
            output_group = "vmlinux")

See discussion thread:

  bazelbuild/bazel#281

Bug: 202075567
Bug: 202075499
Test: builds

Change-Id: I92d5c7f357d2d69c40039ef35b3e0a25cdb7214f
NopeNopeGuy pushed a commit to NopeNopeGuy/kernel_build that referenced this issue Jun 1, 2024
To support select() properly, the argument "outs" in
kernel_build macro cannot be parsed, and it needs to be
passed to _kernel_build directly. Hence, outs (and
for consistency, module_outs and implicit_outs) of _kernel_build
are changed to a string_list.

Then inside _kernel_build_impl, declare_file() of the proper
output file. Because _kernel_build_impl already explicitly
return DefaultInfo(), this is okay.

To continue to support e.g. "kernel_aarch64/vmlinux", filegroups
with output groups are created to point to the correct file:

  kernel_build(name = "kernel_aarch64", out = ["vmlinux"])

becomes

  _kernel_build(name = "kernel_aarch64", ...)
  filegroup(name = "kernel_aarch64/vmlinux",
            srcs = ["kernel_aarch64"],
            output_group = "vmlinux")

See discussion thread:

  bazelbuild/bazel#281

Bug: 202075567
Bug: 202075499
Test: builds

Change-Id: I92d5c7f357d2d69c40039ef35b3e0a25cdb7214f
NopeNopeGuy pushed a commit to NopeNopeGuy/kernel_build that referenced this issue Jun 1, 2024
To support select() properly, the argument "outs" in
kernel_build macro cannot be parsed, and it needs to be
passed to _kernel_build directly. Hence, outs (and
for consistency, module_outs and implicit_outs) of _kernel_build
are changed to a string_list.

Then inside _kernel_build_impl, declare_file() of the proper
output file. Because _kernel_build_impl already explicitly
return DefaultInfo(), this is okay.

The downside is that the following becomes invalid:

  kernel_build(name = "kernel", out = ["vmlinux"])
  filegroup(name = "foo", srcs = ["kernel/vmlinux"])

This is because `kernel/vmlinux` is no longer in the explicit output
list.

See discussion thread:

  bazelbuild/bazel#281

Bug: 202075567
Bug: 202075499
Test: builds

Change-Id: I92d5c7f357d2d69c40039ef35b3e0a25cdb7214f
NopeNopeGuy pushed a commit to NopeNopeGuy/kernel_build that referenced this issue Jun 1, 2024
To support select() properly, the argument "outs" in
kernel_build macro cannot be parsed, and it needs to be
passed to _kernel_build directly. Hence, outs (and
for consistency, module_outs and implicit_outs) of _kernel_build
are changed to a string_list.

Then inside _kernel_build_impl, declare_file() of the proper
output file. Because _kernel_build_impl already explicitly
return DefaultInfo(), this is okay.

The downside is that the following becomes invalid:

  kernel_build(name = "kernel", out = ["vmlinux"])
  filegroup(name = "foo", srcs = ["kernel/vmlinux"])

This is because `kernel/vmlinux` is no longer in the explicit output
list.

See discussion thread:

  bazelbuild/bazel#281

Change-Id: I92d5c7f357d2d69c40039ef35b3e0a25cdb7214f
NopeNopeGuy pushed a commit to NopeNopeGuy/kernel_build that referenced this issue Jun 1, 2024
To support select() properly, the argument "outs" in
kernel_build macro cannot be parsed, and it needs to be
passed to _kernel_build directly. Hence, outs (and
for consistency, module_outs and implicit_outs) of _kernel_build
are changed to a string_list.

Then inside _kernel_build_impl, declare_file() of the proper
output file. Because _kernel_build_impl already explicitly
return DefaultInfo(), this is okay.

The downside is that the following becomes invalid:

  kernel_build(name = "kernel", out = ["vmlinux"])
  filegroup(name = "foo", srcs = ["kernel/vmlinux"])

This is because `kernel/vmlinux` is no longer in the explicit output
list.

See discussion thread:

  bazelbuild/bazel#281

Bug: 202075567
Bug: 202075499
Test: builds

Change-Id: I92d5c7f357d2d69c40039ef35b3e0a25cdb7214f
NopeNopeGuy pushed a commit to NopeNopeGuy/kernel_build that referenced this issue Jun 1, 2024
To support select() properly, the argument "outs" in
kernel_build macro cannot be parsed, and it needs to be
passed to _kernel_build directly. Hence, outs (and
for consistency, module_outs and implicit_outs) of _kernel_build
are changed to a string_list.

Then inside _kernel_build_impl, declare_file() of the proper
output file. Because _kernel_build_impl already explicitly
return DefaultInfo(), this is okay.

The downside is that the following becomes invalid:

  kernel_build(name = "kernel", out = ["vmlinux"])
  filegroup(name = "foo", srcs = ["kernel/vmlinux"])

This is because `kernel/vmlinux` is no longer in the explicit output
list.

See discussion thread:

  bazelbuild/bazel#281

Bug: 202075567
Bug: 202075499
Test: builds

Change-Id: I92d5c7f357d2d69c40039ef35b3e0a25cdb7214f
NopeNopeGuy pushed a commit to NopeNopeGuy/kernel_build that referenced this issue Jun 1, 2024
To support select() properly, the argument "outs" in
kernel_build macro cannot be parsed, and it needs to be
passed to _kernel_build directly. Hence, outs (and
for consistency, module_outs and implicit_outs) of _kernel_build
are changed to a string_list.

Then inside _kernel_build_impl, declare_file() of the proper
output file. Because _kernel_build_impl already explicitly
return DefaultInfo(), this is okay.

The downside is that the following becomes invalid:

  kernel_build(name = "kernel", out = ["vmlinux"])
  filegroup(name = "foo", srcs = ["kernel/vmlinux"])

This is because `kernel/vmlinux` is no longer in the explicit output
list.

See discussion thread:

  bazelbuild/bazel#281

Bug: 202075567
Bug: 202075499
Test: builds

Change-Id: I92d5c7f357d2d69c40039ef35b3e0a25cdb7214f
NopeNopeGuy pushed a commit to NopeNopeGuy/kernel_build that referenced this issue Jun 1, 2024
To support select() properly, the argument "outs" in
kernel_build macro cannot be parsed, and it needs to be
passed to _kernel_build directly. Hence, outs (and
for consistency, module_outs and implicit_outs) of _kernel_build
are changed to a string_list.

Then inside _kernel_build_impl, declare_file() of the proper
output file. Because _kernel_build_impl already explicitly
return DefaultInfo(), this is okay.

The downside is that the following becomes invalid:

  kernel_build(name = "kernel", out = ["vmlinux"])
  filegroup(name = "foo", srcs = ["kernel/vmlinux"])

This is because `kernel/vmlinux` is no longer in the explicit output
list.

See discussion thread:

  bazelbuild/bazel#281

Change-Id: I92d5c7f357d2d69c40039ef35b3e0a25cdb7214f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants