Skip to content

feat: use macros for defining external repositories #146

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

Closed
wants to merge 1 commit into from
Closed

feat: use macros for defining external repositories #146

wants to merge 1 commit into from

Conversation

sudoforge
Copy link
Contributor

This patch adds macros that wrap around this workspace's external
dependencies:

  - `//:repositories.bzl%copybara_repositories`
  - `//:repositories.go.bzl%copybara_go_repositories`
  - `//:repositories.maven.bzl%copybara_maven_repositories`

The primary motivation for this is to enable easier loading of copybara
from within external repositories. Prior to this patch, those external
repositories would need to re-define each and every dependency listed in
this repository's WORKSPACE file. With this patch, those external
repositories need only to load and call the macros (shown above).

An empty BUILD file is also added in this patch as it is necessary in
order to refer to the files in the workspace root.

closes #73

@google-cla google-cla bot added the cla: yes label Nov 30, 2020
@sudoforge
Copy link
Contributor Author

sudoforge commented Nov 30, 2020

I can't see why the import/copybara status check is failing, and can only imagine that the migration contains references or patches to the WORKSPACE file that fail due to this change. I think a Googler will have to fix that, but please let me know if there's anything I can do to help.

While this is functional, I think it would be best to add rules for executing migrations via different targets. I've created another issue to track this effort: #147

@sudoforge
Copy link
Contributor Author

One suggestion I have is to rename this workspace to something more canonical in the Bazel ecosystem -- something along the lines of io_copybara or com_google_copybara. If the maintainers agree with this, I'll happily make the changes within this PR (in a separate commit from the addition of the macros).

@mikelalcon
Copy link
Collaborator

Hi!

Thanks for the contribution. The reason for the failure is that the new BUILD file doesn't contain the Apache License header. Would you mind adding it? Thanks!!

Re workspace rename: Sounds good to me. Probably better to put it in a different PR, as we merge each PR in a single commit internally.

Thanks again!

@sudoforge
Copy link
Contributor Author

sudoforge commented Nov 30, 2020

Added the license header in //:BUILD.

I created a separate issue for tracking the workspace renaming (#149) and will take care of sending that along shortly after this is merged.

Edit: After thinking on it a bit, I think it would make more sense to merge that change in (#150) prior to this.

@sudoforge
Copy link
Contributor Author

Hmm, seems like there might be an additional error to check in the import/copybara build job -- let me know if there are additional changes necessary here.

@sudoforge
Copy link
Contributor Author

sudoforge commented Dec 3, 2020

Checking in -- if a maintainer can let me know what the error with the import/copybara status check is, I'll take care of fixing that.

Also going to run through the dep macros and see if there's anything that isn't necessary for external consumers (and move them to the workspace)

@sudoforge
Copy link
Contributor Author

@mikelalcon sorry to ping you directly, but when you have a chance next week, can you please check the import/copybara job and let me know what needs to be changed this side, if anything?

@mikelalcon
Copy link
Collaborator

Hi!

Sorry for the late response!The reason is the new BUILD file in the root that we don't want to import. I've sent an internal change to fix it. Could you try again?

Thanks!

@sudoforge
Copy link
Contributor Author

sudoforge commented Dec 23, 2020

Was travelling earlier today, but I've rebased on top of master and force-pushed in order to (update the tree, and) trigger a re-run of the status checks.

@tsawada
Copy link
Contributor

tsawada commented Apr 30, 2021

+1! This would greatly improve the usability in my use case. Is there anything blocking?

This patch adds macros that wrap around this workspace's external
dependencies:

  - `//:repositories.bzl%copybara_repositories`
  - `//:repositories.go.bzl%copybara_go_repositories`
  - `//:repositories.maven.bzl%copybara_maven_repositories`

The primary motivation for this is to enable easier loading of copybara
from within external repositories. Prior to this patch, those external
repositories would need to re-define each and every dependency listed in
this repository's WORKSPACE file. With this patch, those external
repositories need only to load and call the macros (shown above).

An empty BUILD file is also added in this patch as it is necessary in
order to refer to the files in the workspace root.

closes #73
@sudoforge
Copy link
Contributor Author

@tsawada as far as I know, the only thing that this CL needs is a review and merge by a Copybara maintainer.

@sudoforge
Copy link
Contributor Author

@mikelalcon could I bother you to take a look at the internal job that's failing?

@sudoforge sudoforge deleted the 73/add-macros-for-external-deps branch January 12, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants