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

Allow http_archive, git_repository and local_repository to take optional build_file/build_file_content to behave like new_* when specified #2037

Closed
ittaiz opened this issue Nov 3, 2016 · 27 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) type: feature request
Milestone

Comments

@ittaiz
Copy link
Member

ittaiz commented Nov 3, 2016

Following the discussion on #1952 I want to suggest renaming new_* rules to bazel_augmenting_* or decorate_with_bazel_* (so for example new_http_archive would be decorate_with_bazel_http_archive).
I know this can be a costly change but as someone who has had to lookup what new_* means several times I think it can be valuable in the long run. Fortunately Bazel isn't 1.0 yet :)

@laszlocsomor
Copy link
Contributor

Frankly I'm happier with the new_ prefix than a long one, because (a) I'd have to look up bazel_augmenting_* just as much as new_, so no time saved there, (b) once you know what it means it's faster to type the short prefix than the long one.

@kchodorow : WDYT?

@laszlocsomor
Copy link
Contributor

@ittaiz : Plus changing these names would mean everyone who already uses these rules would have to update their WORKSPACE files.

@ittaiz
Copy link
Member Author

ittaiz commented Nov 4, 2016

Hi,
I understand the costs but I think this is exactly something you can do
before 1.0
The examples I gave were just a rough attempt, another one can be
bazelify_http_archive or something else. I understand you remember after
you've looked it up but I don't and I've looked it up multiple times. I
think this has value but maybe this question deserves a wider forum
On Fri, 4 Nov 2016 at 15:49 László Csomor notifications@github.com wrote:

@ittaiz https://github.com/ittaiz : Plus changing these names would
mean everyone who already uses these rules would have to update their
WORKSPACE files.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#2037 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIF2cYPithb9ictKnVr3TrRECUN93Rks5q6zfbgaJpZM4Koxiw
.

@laszlocsomor
Copy link
Contributor

Sure, do you want to send an email to bazel-discuss@googlegroups.com ?

@petemounce
Copy link
Contributor

It would make it easier to google-search to find out how to debug/fix-usage-of/apply the rules.

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Nov 4, 2016

@petemounce : why? you can search "bazel new_local_repository" and the first hit is the build encyclopedia (both on Google and Bing). I'm not convinced that a more descriptive verbose name would make it unnecessary to look up the rule's semantics, simply because the concept of the rule is non-trivial.

WDYT?

@ittaiz
Copy link
Member Author

ittaiz commented Nov 4, 2016

I already gave my opinion in general but I just want to add I think the
concept is pretty trivial. It's an adapter from the GoF. From one build
system API to Bazel API.
On Fri, 4 Nov 2016 at 17:30 László Csomor notifications@github.com wrote:

@petemounce https://github.com/petemounce : why? you can search "bazel
new_local_repository" and the first hit is the build encyclopedia (both on
Google and Bing). I'm not convinced that a more descriptive name would make
it unnecessary to look up the rule's semantics, simply because the concept
of the rule is non-trivial.

WDYT?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2037 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIFx3C87zTsneA7rtMOZ2zmWgAqfgzks5q6098gaJpZM4Koxiw
.

@laszlocsomor
Copy link
Contributor

I sent this question to bazel-discuss@

@petemounce
Copy link
Contributor

@laszlocsomor Finding the docs is easy. Making the names more unique would make it easier to find other peoples' problems and usages in blog posts, stackoverflow, github, etc.

@jin
Copy link
Member

jin commented Nov 4, 2016

As a beginner to Bazel, I remember trying to figure out the difference between new and non-new. It didn't take long to realize that it's a convention associating with a new BUILD file, therefore "bazel-ifying" a repository.

FWIW: I think build_ is a good alternative to new_. build_local_repository gives a context that you're building a local_repository by injecting a BUILD file.

@ianoc-stripe
Copy link

I'd be of the opinion its not worth the churn to change the name. For any of the suggested names i'd wind up googling what it means/does and and reading about it in the bazel docs. The only benefit here seems to be for looking up stack overflow questions, that doesn't seem to me personally enough to justify such a breaking change.

@pcj
Copy link
Member

pcj commented Nov 4, 2016

I would consider this an educational issue and not a naming issue. I prefer the concise new_ and agree does not warrant change. A figure graphic in the documentation would be useful to illustrate the concept however.

I tried to explain this in section 1.4.3: Workspace Rules that accept a BUILD file as an argument of a recent blog.

@lberki
Copy link
Contributor

lberki commented Nov 7, 2016

new_ is an unfortunate name, but I don't think the benefits of changing it outweigh either the costs of breaking practically everyone or maintaining aliases forever.

@dfabulich
Copy link
Contributor

https://bazel.build/versions/master/docs/be/workspace.html shows three new_* rules corresponding to three unprefixed rules.

git_repository / new_git_repository
http_archive / new_http_archive
local_repository / new_local_repository

In all three cases, they do exactly the same thing, except the unprefixed name requires a pre-existing BUILD file, whereas the prefixed_name requires the WORKSPACE to provide a build_file or build_file_content. I'm struggling to identify a good/clear name for this distinction, and I agree that "new" doesn't convey the concept.

What if, instead, the unprefixed rules could handle both modes? The easiest way to implement that would be to add add build_file/_content parameters to the unprefixed rules; if they were unspecified, the rule would require a pre-existing BUILD file in the imported archive/repository. The new_* rules would be deprecated and perhaps deleted in a future version.

@kchodorow
Copy link
Contributor

@dfabulich good idea, we're going to go with your proposal.

@ittaiz
Copy link
Member Author

ittaiz commented Nov 19, 2016

Indeed better than my initial suggestion :)
On Fri, 18 Nov 2016 at 17:36 Kristina notifications@github.com wrote:

@dfabulich https://github.com/dfabulich good idea, we're going to go
with your proposal.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2037 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABUIFz5wvOC9Ny2fYQBjBUIo1UerxB1kks5q_cX4gaJpZM4Koxiw
.

@damienmg
Copy link
Contributor

damienmg commented Dec 5, 2016

Ok closing since:

  1. The new proposal is implemented,
  2. We are going to deprecate new_* when we move to skylark.

@damienmg damienmg closed this as completed Dec 5, 2016
@ittaiz
Copy link
Member Author

ittaiz commented Dec 5, 2016 via email

@damienmg
Copy link
Contributor

damienmg commented Dec 6, 2016

Humm I thought the CL was in. Apparently not.

Let's reopen this issue to track it.

@damienmg damienmg reopened this Dec 6, 2016
@damienmg damienmg changed the title Rename new_* to something more meaningful Allow http_archive, git_repository and local_repository to take optional build_file/build_file_content to behave like new_* when specified Dec 6, 2016
@damienmg damienmg added P2 We'll consider working on this in future. (Assignee optional) type: feature request labels Dec 6, 2016
@damienmg damienmg added this to the 0.6 milestone Dec 6, 2016
@damienmg
Copy link
Contributor

damienmg commented Dec 6, 2016

So there are skylark drop-in replacement that does that but we should do it for the native rules too.

@davidzchen
Copy link
Member

If we are going to make the change to the native rules, how about let's also introduce the new urls attribute to the native rules so that users can make use of the fallback URL feature that @jart implemented and to make the transition to the Skylark rules smoother?

@jart
Copy link
Contributor

jart commented Dec 14, 2016

The deed is already done. All the native workspace rules support multiple urls, except for maven_jar and git_repository.

@davidzchen
Copy link
Member

Ah that's right. Thanks.

To help with the migration, how about if we print a deprecation warning to encourage people to use urls if they are still using url?

@jart
Copy link
Contributor

jart commented Dec 17, 2016

I don't feel strongly enough about it to put a deprecation warning on it. I marked it as deprecated in the docs. I would even go so far as to remove documentation for the url attribute. Whether or not a warning is put in place, I'll leave to the Bazel team.

@dslomov dslomov added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) external-repos-triaged and removed P2 We'll consider working on this in future. (Assignee optional) labels Jan 11, 2018
@mboes
Copy link
Contributor

mboes commented Mar 17, 2018

@damienmg said in 2016:

Ok closing since:The new proposal is implemented,

But that turned out incorrect at the time:

Humm I thought the CL was in. Apparently not.

What about now (a year and a half later)? If the aforementioned CL exists, has it been merged?

@laszlocsomor
Copy link
Contributor

Damien no longer works on the team.
/cc @dslomov to triage

@dslomov
Copy link
Contributor

dslomov commented Mar 21, 2019

All these rules are gone now,

@dslomov dslomov closed this as completed Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) type: feature request
Projects
None yet
Development

No branches or pull requests