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.

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.

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. :)

Comment on attachment 8890487 [details] Bug 1384680 - Generalize GRADLE_MAVEN_REPOSITORY to GRADLE_MAVEN_REPOSITORIES. https://reviewboard.mozilla.org/r/161628/#review167524

Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/781e8d0045d0 Generalize GRADLE_MAVEN_REPOSITORY to GRADLE_MAVEN_REPOSITORIES. r=gps,sebastian

This can't have impacted talos.