Closed Bug 1384680 Opened 7 years ago Closed 7 years ago

Generalize GRADLE_MAVEN_REPOSITORY to GRADLE_MAVEN_REPOSITORIES

Categories

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(1 file)

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1380639#c13, the future will require us to proxy multiple Gradle Maven repositories when collecting Gradle dependencies into our toolchains.  This ticket tracks supporting multiple repositories.
Blocks: 1384695
Comment on attachment 8890487 [details]
Bug 1384680 - Generalize GRADLE_MAVEN_REPOSITORY to GRADLE_MAVEN_REPOSITORIES.

https://reviewboard.mozilla.org/r/161628/#review167120

::: mobile/android/gradle.configure:55
(Diff revision 1)
> +option(env='GRADLE_MAVEN_REPOSITORIES',
> +       nargs='+',
> +       default=('https://jcenter.bintray.com/',
> +                'https://maven.google.com/',
> +       ),
> +       help='Comma-separated URLs of Maven repositories containing Gradle dependencies.')

To be overly pedantic, a comma is a safe character in URLs. This means that if you split the string on commas, you may split in the middle of a URL!

It is better to delimit URLs by an unsafe character (like a space) or to pass in each URL as a separate argument to configure.

Also, nowhere in this code do you actually split the argument. So I'm not sure passing multiple comma-delimited URLs actually works!

::: mobile/android/gradle.configure:60
(Diff revision 1)
> +       help='Comma-separated URLs of Maven repositories containing Gradle dependencies.')
> +
> +@depends('GRADLE_MAVEN_REPOSITORIES')
> +@imports(_from='os.path', _import='isdir')
> +def gradle_maven_repositories(values):
> +    if not len(values):

I don't think there is a need to `len()` here as an empty option should evaluate as false.
Attachment #8890487 - Flags: review?(gps) → review-
Comment on attachment 8890487 [details]
Bug 1384680 - Generalize GRADLE_MAVEN_REPOSITORY to GRADLE_MAVEN_REPOSITORIES.

https://reviewboard.mozilla.org/r/161628/#review167120

> To be overly pedantic, a comma is a safe character in URLs. This means that if you split the string on commas, you may split in the middle of a URL!
> 
> It is better to delimit URLs by an unsafe character (like a space) or to pass in each URL as a separate argument to configure.
> 
> Also, nowhere in this code do you actually split the argument. So I'm not sure passing multiple comma-delimited URLs actually works!

So, multiple comma-delimited URLs definitely works, both locally and on try.  I don't understand _how_ it works, but it does.  I assume something with `nargs='+'` is trying to interpret the argument as a Python structure.

I thought about which character to split on, and decided that the likelihood of a URL we care about including a comma is low enough that it was better to roll with how the build system works right now.

In light of this, will you reconsider?

> I don't think there is a need to `len()` here as an empty option should evaluate as false.

Hmm.  Other places in this file suggested that `values` and `values[0]` would be different.  I'll try `if not values` and land with this if it's equivalent.
Comment on attachment 8890487 [details]
Bug 1384680 - Generalize GRADLE_MAVEN_REPOSITORY to GRADLE_MAVEN_REPOSITORIES.

https://reviewboard.mozilla.org/r/161628/#review167120

> So, multiple comma-delimited URLs definitely works, both locally and on try.  I don't understand _how_ it works, but it does.  I assume something with `nargs='+'` is trying to interpret the argument as a Python structure.
> 
> I thought about which character to split on, and decided that the likelihood of a URL we care about including a comma is low enough that it was better to roll with how the build system works right now.
> 
> In light of this, will you reconsider?

It must be option parsing then. Ugh, magic.

I agree a comma in one of these URLs is likely unlikely. I just wanted to make you aware of the potential limitation.

> Hmm.  Other places in this file suggested that `values` and `values[0]` would be different.  I'll try `if not values` and land with this if it's equivalent.

option() values are special Python types that quack like tuples. A special NegativeOptionValue holds no value and evaluates as false. A special PositiveOptionValue may hold no value but defaults to true and evaluates as true.

Since they quack like tuples, that is why many consumers use [0] indexing.

Since this option takes string arguments and has a default, non-boolean value, I /think/ you can get away with just testing truthiness. But you may want to run it by glandium to be sure. The option types are a bit wonky and I've introduced subtle bugs touching that code and consumers of it :/
Comment on attachment 8890487 [details]
Bug 1384680 - Generalize GRADLE_MAVEN_REPOSITORY to GRADLE_MAVEN_REPOSITORIES.

https://reviewboard.mozilla.org/r/161628/#review167236

This is simpler than I thought. :)
Attachment #8890487 - Flags: review?(s.kaspari) → review+
Comment on attachment 8890487 [details]
Bug 1384680 - Generalize GRADLE_MAVEN_REPOSITORY to GRADLE_MAVEN_REPOSITORIES.

https://reviewboard.mozilla.org/r/161628/#review167524
Attachment #8890487 - Flags: review?(gps) → review+
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/781e8d0045d0
Generalize GRADLE_MAVEN_REPOSITORY to GRADLE_MAVEN_REPOSITORIES. r=gps,sebastian
https://hg.mozilla.org/mozilla-central/rev/781e8d0045d0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
No longer blocks: 1384695
This can't have impacted talos.
No longer depends on: 1390824
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 56 → mozilla56
You need to log in before you can comment on or make changes to this bug.