-
Notifications
You must be signed in to change notification settings - Fork 111
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
Added rudimentary support for bzlmod #146
Conversation
Hey Luke! Thanks for all your energy contributing here and figuring this out. I just want to check in: This is enough better than the WORKSPACE approach for you that it's worth bifurcating the install path, yeah? I hear that you're looking to switch, but many high quality-dependencies (plus bazel itself) still require the WORKSPACE, and this seems like about the same install difficulty. If so, I must say, I like this a lot more, actually, than the usual central repository approach. This way we're not submitting--and waiting on-- a million PRs to them every time we're updating. People can still stay up to date with Renovate (I'm pretty sure it now does git override) and we're not brought in transitively by default. Win-win-win. ^ That is to say, I'm a bit torn (please convince me if you think I'm wrong)--but presuming we should support bzlmod at some point, this seems like the way to do it, and I really appreciate your research and work. Assuming you're still convinced of it: I think I should be asking you to take one more pass to see just how good we can get it, and then we'll merge. As a non-exhaustive set of things that seemed worth considering on a quick read: it looks like some of the arguments above are optional and set to their defaults, and if so we should probably omit to keep things simple for people. We probably want some instructions in the main README ("if you'd really like to use bzlmod..."). And could I ask you to straighten me out on how these extensions interact with Bzlmod? I think we probably want the bazel_dep call to declare this as a dev_dependency to avoid it breaking things by being brought in transitively and skipping the override. Thoughts on git_override vs archive_override? (the WORKSPACE advice was always http_archive, but maybe they've fixed the speed issues.) Also, maybe we should be adding any future dependencies directly via bzlmod instead of the extension? What do you think? Thanks again, |
Hey Chris! Thanks for your message! This is my first PR, so definitely appreciate the feedback and pointers! To address the points you've made: Is it worth it better than the WORKSPACE approach?
Downsides
From my perspective and current understanding, this would be the way to integrate bzlmod, and yes, I'll be doing a tidy up pass on this PR to get it to what should be production ready. I think the following points are of important consideration:
Finally, it would be good to consider adding the project to the Bazel Central Registry once we have bzlmod support, it should require just a commit to here, after checking the guidelines and maybe some extra metadata files added to the repo?. That would quite literally bring adding this to a new project down to the single line: One more thought: I've committed the slight code changes that should refine this, let me know your thoughts on all this and if you'd like me to update the readme and get it all ready to be merged, if you do think it's a good idea. |
Tweaked MODULE.bazel
Luke, thanks again for your excellent work and scoping on all this. I really appreciate it. Let's go for it. I'm right there with you and agree on almost everything, but with the following tweaks to my thinking. As always, please push back if you disagree.
Additionally, I love your clever workspace dips unification trick. I've tested that it doesn't crash back to 5.0, which should be old enough for us. Even if we aren't really using it just yet, let's keep it in case we need it. And I know I'm flip flopping, but let's just recommend git_archive if you're finding it plenty fast enough. I tried to quickly check out the bazel implementation, but it's pretty layered, and git_archive should be easier for people because they don't need the second, hash-copying step. Better to recommend just one good way of doing things. Thanks again for all your great work here. I'm hoping you'll do one final round of polish for us on the above, and then I'll do and merge, hopefully not offending you with any edits I make. I can either just merge and then you tell me what could be better async, or I can make the changes I think and then show you before merging, just lmk. Or if you're uncertain about anything I can just do. But not tonight--need to go to bed now. Cheers to helping with the building of great software, |
Hey Chris! For 1) , you're absolutely right, we should definitely be recommending as a dev dependency, I was testing using that but forgot to add it to the comments in those files as I was going to transpose it properly when I did the readme. Will make sure it's done like that! For 2), that makes sense to steer clear of the repository, and if renovate is working with git override that should be perfect. git override is also neater than archive override as we don't have to strip paths or anything, so yes, I agree let's just recommend that way only in the readme. The strongest argument I had for the repository was that I was under the impression there'd be a transitive dependencies issue using the overrides - let me explain:
would be ignored in the dependency project, leaving just the Great to hear you liked the unification trick, and thank you so much for backwards testing it! Definitely agree with sticking to just git override, I actually didn't get the archive hash working on bzlmod yet either, I think they changed the syntax a bit because sha256 = "hash" isn't accepted, and it gives me invalid hash with integrity = "hash", even though the verbose message says to add the former. Probably not fully documented or I missed it. I'll definitely go through with another commit, probably tomorrow or as soon as I get a chance, then let me know if there's any other thoughts you've had, otherwise of course go for gold tweaking it before merge, as I think I said earlier, I'm still pretty new to big production environments and community code, so I'd love to get the feedback via any changes you make so I know what to aim for next time! Thanks for all the help with this, I'm really glad I can contribute a little bit, this project is exactly what I needed for a bunch of my other works and I massively appreciate what you guys have done for the community! |
Will also set up and try out renovate when I get some free time, will confirm with you that it all works with this setup! |
Really appreciate your tracking down the edge case and sharing! If you still have the project handy, one more key thing to check would be to make sure there's not an error if the top level project doesn't use this tool, but one of the dependents does (as a dev_dependency). I'd hope they got that right, and if so, we're in the clear--with a very clever, better solution by you :) [I'm assuming the sha/integrity hash stuff is just rough edges because bzlmod is still prerelease, but I appreciate your giving me a heads for the future. Looks like they're tracking at https://github.com/bazelbuild/bazel/issues/17803, which I've subscribed to. Linking because you might want to, too.] If useful to you, here's an example renovate config from one of our other projects: https://github.com/hedronvision/bazel-cc-filesystem-backport/blob/main/renovate.json5 Forward looking: Sounds like a plan! You're already great and thorough and exploratory and just generally awesome to work with. So continue with confidence :) Thanks for being great! |
Good point to check, I've just tested now, and as expected, having it as a dev dependency makes all the difference. If it's in the sub-project without being a dev dep (and the parent project doesn't use it), the override gets ignored but the dep isn't found and throws an error. Once dev dep is enabled, it just gets ignored entirely in the sub project which is what we'd want! Thanks for linking that issue too, that's exactly what I found happened when I tried as well 😝 Thank you for linking to the renovate config too, I'll have a go at setting that up today! On that note, do you have any suggestions of tools I should look into for c++ development? I was looking for some new ways to do things (which led me here in the first place) and am now using vscode, Bazel + this, clangd, sonarlint, and will shortly learn renovate and get into github actions. Is there anything you'd recommend I look into that could be handy? |
Also quick question for the readme, for a git url, is it better to use the https:// or the git@github.com as the remote? Both work, but I guess https would be more universal than ssh? |
Yay! Thank you! Tools: Depends a bit on what you're trying to build! Some things I like, but that aren't too C++ specific are:
I think https--I know GitHub has switched over to steering people towards that for a variety of reasons. |
Thank you so much for those! I've committed the most recent changes we've discussed now, let me know if there's anything else you think I should do! |
I gave it a once-over to see what polish I could add! Will tap out what went through my head and then merge. |
Looks perfect! Just looked through your changes, makes sense and definitely cleared up documentation🤝👌 |
A good chunk of the commit was me improving older mistakes of mine that I happened to see while reading through :) I moved the local development stuff to the ImplementationReadme, since it seemed like it fit even better there.
I generally made any improvements to the comment or structure that occurred while I was reading through. I'm sure they could be even better still, so please, don't hesitate to toss up changes when you see improvements. And (in a bit of a change of heart) I moved the bzlmod instructions ahead of the WORKSPACE ones. Thanks for pathfinding us such to a great solution--and again, being a joy to work with. Now let's get you on that contributor board! |
Actually, could I ask you to look into one more thing for me: If you remove the If not, I think we should cut that, since we're living at head rather than using versioning. Obviously, if that'd break things, then let's leave it :) I imagine it breaks, which is why you added it--but it'd be awesome if we could also add a comment noting that. |
That's really helpful to see your thought pattern with the edits, thanks! I do agree that the info you moved definitely belongs in the ImplementationReadme, but I can also say I completely didn't know about that doc until seeing your commit😅 Not sure if there's a good way to improve visibility, although I'm sure I would've found it eventually haha. I was also a bit surprised about seeing the order swapped on bzlmod vs workspace😆, but I do agree, anyone setting up an entirely new project and adding this probably will be considering getting right into bzlmod, so it fits well :) As for the version tag, I definitely had a think and play around with this, and what I found was: Thoughts? Also, I just did a quick test and it looks like the MODULE.bazel actually works without the version number, so maybe we should remove it from there too, and just go for a no versioned releases methodology until such time as we need to? Alternatively, did you want the MODULE.bazel set to version 1.0.0 maybe and in the docs we add the import statement with 1.0.0, then we can increment if we ever add to central repository?🤷♀️🤷♀️ Edit: Sorry, I read your original post to be about the removal of the version number from the import statement |
Thanks for experimenting around--and sorry for being ambiguous. Sounds like that works, so I'll pull it out and merge! |
Yay! Luke, welcome to the contributors board and a huge congratulations on your first ever open source contribution. You did it in in style, scoping a new feature, coming up with a significantly better approach, and then bringing it to completion. That's a pretty huge milestone. You should be proud of yourself! Thanks again for being great to work with. Hopefully it's empowering and the first of many! Know that you're always welcome here and that I really appreciate it. Please don't be a stranger. Thanks for leaving things better than you found them! |
Thanks so much Chris, It's been so great working with you, and I'm so glad I could make a contribution to this awesome project! Cheers to open source, thank you, and happy developing! Catch you again soon for future improvements! |
This commit is my rudimentary application of the Bazel docs to implement support for the bzlmod system. It should work alongside the existing system, but provide FAR easier access for people using bzlmod to add this repo's functionality to their project. It is as simple as adding the following to a MODULE.bazel file:
Resolves #54