Skip to content
This repository has been archived by the owner on Mar 17, 2024. It is now read-only.

Add alternative implementation #89

Merged
merged 1 commit into from Nov 19, 2021
Merged

Add alternative implementation #89

merged 1 commit into from Nov 19, 2021

Conversation

cpsauer
Copy link
Contributor

@cpsauer cpsauer commented Nov 18, 2021

@siddharthab, I really appreciate all you've built here over the years. We loved using it and are sad to see it's now in maintenance mode. You were the first recommendation people pointed us to when we migrated to Bazel. Thank you for all you've done!

For our projects that involved platform configuration and Bazel's compiler wrappers (like in #57) we'd initially started experimenting with action_listeners (like Kythe; slow, requiring a full build), after noticing some limiting problems with the aspect-based approach. We then noticed that we could get something orders of magnitude faster and equally accurate by directly asking bazel for its compile commands using aquery, and then doing some subsequent "debazeling" to let clang tooling understand it.

The efforts to fix those issues led to https://github.com/hedronvision/bazel-compile-commands-extractor, which we've just released publicly after seeing that you were moving this repo into maintenance mode. (Also because lots of people continued requesting that we release it.)

Anyway, we'd be honored if you'd consider adding us to your list of alternatives. The effort was certainly inspired by yours, and I think the aquery approach is great one if you're ready to move on and pass the torch.

Thanks for your consideration,
Chris

P.S. Also, congratulations on all the great stuff you all have built at Grail--and on the IPO-turned-acquisition. It's been fun to get to watch from the sidelines.

@siddharthab siddharthab merged commit a6374bc into grailbio:master Nov 19, 2021
@siddharthab
Copy link
Collaborator

Thank you for your kind words, and thank you for showing so much rigor in what would otherwise be just some uninteresting tooling project. This definitely feels like a passing the baton/torch moment. My best wishes for everything you do in life.

@@ -16,6 +16,8 @@ give you write access.

#### Alternative/Derived implementations:

- https://github.com/hedronvision/bazel-compile-commands-extractor
- Uses an alternate approach based on aquery. This directly asks Bazel for its exact build commands, while still being fast and not requiring a full build. Avoids whole categories of issues.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpsauer Can you elaborate on "whole categories of issues" here? (we're trying to evaluate different solutions and would like to understand this)

Copy link
Contributor Author

@cpsauer cpsauer Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @thii! Good to hear from you again--it's been a bit since we interacted on the interwebs, but I still really appreciate your help with, I think, tulsi stuff, back when I was fixing some of the external workspace things there.

Sure; happy to. But it's been a little bit since I first wrote that, so I'm dredging up from memory. I do remember we prototyped implementing it a few different ways and found aquery to be the best by far. IIRC, the reasoning for aquery vs aspects/query is that, fundamentally the database is a listing of command line actions, which is direct mapping with aquery, rather than crawling the graph of targets you'd get with aspects/query. With aspects/query, there were lots of problems and additional lift required to convert things into commands correctly, whereas you get the exact command for free with aquery. In particular, you'd otherwise have to duplicate the attr->command line logic from bazel & the toolchain, and IIRC that gets pretty brittle with custom flags and toolchains and on other platforms, especially with in-graph configuration transitions for cross-compiling. Aquery gets all that basically for free by looking directly at the compile command, though you do need to do some light de-bazeling when Bazel use wrapper scripts around the compiler drivers, like on Apple platforms. Aquery can also easily filter to Cpp/Objc compile actions, even for actions that are for code-gen tools being built for the host, and I think that's not so easy with the other techniques. [We also built a working version based on action_listeners, which are now deprecated by bazel, but that too was worse. Happy to explain more if you're curious, but I think you're probably evaluating vs the aspect/query approach done here.]

I'm curious what you're evaluating/building!

Copy link

@thii thii Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the context. Yes, we are evaluating aspects vs aquery. We haven't had the above issues with aspects, but those are definitely something we'll keep in mind. However, we found issues with with the aquery approach so far:

  • Since aquery only does an analysis, you can't get correct compile commands for undetermined sources, e.g. when you have an input that is a tree artifact.
  • The generated compilation database can reference to not-yet-existent source files (you need to do a full build to get those files). With aspects, you can request generated source files while generating the compilation database, without doing a full build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 For this or for some other use case?
The latter might be solvable with a complementary aspect, at least. Any chance you have an example of the first for me to think about?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# rule.bzl
def _impl(ctx):
    dir_name = ctx.attr.name + ".c"
    out = ctx.actions.declare_directory(dir_name)
    ctx.actions.run(
        outputs = [out],
        executable = ctx.executable._tool,
        arguments = [
            ctx.bin_dir.path,
            dir_name,
        ],
    )
    return [DefaultInfo(files = depset([out]))]

codegen = rule(
    _impl,
    attrs = {
        "_tool": attr.label(
            executable = True,
            cfg = "exec",
            default = "//:tool.sh",
            allow_single_file = True,
        ),
    },
)
# BUILD
load(":rule.bzl", "codegen")

codegen(name = "srcs")

cc_library(
    name = "a",
    srcs = [":srcs"],
)
# tool.sh
cd $1
mkdir -p $2
cd $2
touch inner-1.c
touch inner-2.c

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this or for some other use case?

Not sure what this question is about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ I'm just curious what you're working on that's motivating the questions. Are you thinking about making a change to this repo--or building a different tool entirely, maybe for something else?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll DM you on Slack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 but could I get you to email me instead to dm? (Migrating computers--slack currently borked) christophersauer -at- pacbell.net

Copy link
Contributor Author

@cpsauer cpsauer Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thii, bumping the above just bc I think there's a good chance I lost your message here. Thanks so much for starting the discussion and for the example, above!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants