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

Refactor WORKSPACE #445

Closed
wants to merge 1 commit into from
Closed

Refactor WORKSPACE #445

wants to merge 1 commit into from

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Apr 9, 2021

Trying to make it more readable. I've been considering adding some more repositories, but I'm pausing because it seems to be getting really messy.

@jonmeow jonmeow requested a review from chandlerc April 9, 2021 22:31
@jonmeow jonmeow requested a review from a team as a code owner April 9, 2021 22:31
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Apr 9, 2021
@chandlerc
Copy link
Contributor

I'm not really following the factoring you're aiming for so far...

Curious what the problem is that you're hitting and what the overall structure you're aiming for is? Splitting everything up into the _init and _load, for me, isn't helpful. there is a lot of implicit state between them and no obvious connection.

I could see just factoring out into separate files for general areas -- Python, C++ toolchain-y stuff, LLVM-y stuff, Bison/Flex-y stuff. But I'd just use single files and macros for that?

But worried I'm just not understanding the problem you're trying to solve that motivates one factoring over another.

Relatedly -- what other repositories? Anything I can help with?

@jonmeow
Copy link
Contributor Author

jonmeow commented Apr 9, 2021

A bzl file needs to have load() at the top, and you can't load before the repository exists. (thus the bzl file needs to be split, or you need to not have repositories in bzl files)

I've been considering adding repositories for botli, woff2, and re2 as I work towards the conversion tooling.

@chandlerc
Copy link
Contributor

A bzl file needs to have load() at the top, and you can't load before the repository exists. (thus the bzl file needs to be split, or you need to not have repositories in bzl files)

Maybe its just me, but I find having everything spread out across so many files harder to keep track of than everything being lumped together into the WORKSPACE file.

That said, there might be a different way to factor things that would work better using local_repositorys in the top-level WORKSPACE rather than repository rules... I may take a look and see if I see a way to make that work.

I've been considering adding repositories for botli, woff2, and re2 as I work towards the conversion tooling.

What do you think about just checking them into our third_party directory? Given that we'll likely want to patch and compare things, it seems like even submodules might not make much sense, and instead just dropping them directly into the repository pinned to specific version might be fine...

@jonmeow
Copy link
Contributor Author

jonmeow commented Apr 12, 2021

A bzl file needs to have load() at the top, and you can't load before the repository exists. (thus the bzl file needs to be split, or you need to not have repositories in bzl files)

Maybe its just me, but I find having everything spread out across so many files harder to keep track of than everything being lumped together into the WORKSPACE file.

That said, there might be a different way to factor things that would work better using local_repositorys in the top-level WORKSPACE rather than repository rules... I may take a look and see if I see a way to make that work.

Yes, I think we differ here: for me, the WORKSPACE is already hitting the tipping point of unreadability. This is a bit like large functions: I interpret it as a long, linear ordering with no obvious rationale for that.

Anyways, given there isn't consensus that this needs to be cleaned up yet, I'll abandon this change.

I've been considering adding repositories for botli, woff2, and re2 as I work towards the conversion tooling.

What do you think about just checking them into our third_party directory? Given that we'll likely want to patch and compare things, it seems like even submodules might not make much sense, and instead just dropping them directly into the repository pinned to specific version might be fine...

Both brotli and re2 provide WORKSPACE files. Regardless of how they're pulled in, they would still need an entry (directly or indirectly) in our workspace.

@jonmeow jonmeow closed this Apr 12, 2021
@mmdriley
Copy link
Contributor

fwiw I'm very sympathetic to @jonmeow 's view here -- it's hard for me to follow the WORKSPACE file because it does so many different things at the same level/scope. I wish we could put all the M4-related stuff in one place, Python stuff somewhere else, etc. Maybe separate files is too heavyweight, but wow I wish we could use functions.

But I looked at Envoy and gRPC -- two projects that have spent a lot more energy on their Bazel configs than I hope we ever will -- and it doesn't seem like they've done much better:

The pattern seems to be to have a stage that loads everything, then an "extra" stage that calls functions from the loaded modules.

More context on ideas for better WORKSPACE management in Bazel: bazelbuild/bazel#1943

Maybe we can get some value out of separating sections with headers and some whitespace?

@jonmeow jonmeow deleted the refactor-workspace branch April 12, 2021 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants