Variant support

[neverpanic]

Allow passing variants into list-subports by separating port name and variants specification with an "@" sign. @ is not a valid character in port names and should allow us to specify variants per port in the buildbot's portlist.

When not using this new syntax, the output will look exactly as it did before to avoid breaking the existing setup.

This requires reverting f6e4681 due to the reasons explained in a comment on this commit.

See also https://trac.macports.org/ticket/52742.

[ryandesign]

Allow passing variants into list-subports by separating port name and variants specification with an "@" sign.

Why do we need a new syntax? Can't we use the same syntax that port uses? Each arg is either a port name or, if the arg begins with + or -, it's a variant or variants to be applied to the preceding port.

[neverpanic]

Why do we need a new syntax? Can't we use the same syntax that port uses? Each arg is either a port name or, if the arg begins with + or -, it's a variant or variants to be applied to the preceding port.

I did not want to do this since the buildbot configuration currently does not support support this behavior. With the current approach, you could just put gtk3@+quartz into a portlist property and it would do the right thing. If you use spaces, we must modify splitting of the portlist on the buildbot to correctly parse this and also modify processing of the mpbb list-subport output of the buildbot to treat lines as port specs (since it currently just splits at any whitespace).

Of course this then introduces the problem of making this change in a backwards-compatible manner with the buildbot configuration, because we cannot change both at the same time.

[ryandesign]

I see you've done a lot of work on this already so if you don't want to change it I would be happy for now if at least the API exposed to developers via the buildbot web site does not require them to use the @ syntax.

I did not want to do this since the buildbot configuration currently does not support support this behavior. With the current approach, you could just put gtk3@+quartz into a portlist property and it would do the right thing. If you use spaces, we must modify splitting of the portlist on the buildbot to correctly parse this

My only experience with Python is editing our buildbot master.cfg, so it has taken me a long time to understand what you're saying here. We do:

class SetPropertyFromCommandWithPortlist(steps.SetPropertyFromCommand):
    def setBuild(self, build):
        super(SetPropertyFromCommandWithPortlist, self).setBuild(build)

        # support forced build properties
        ports = set(self.getProperty('portlist', default='').split())

        # paths should be category/portdir(/...)
        ports.update(ifilter(None, imap(port_from_path, self.build.allFiles())))

        self.setProperty('fullportlist', ' '.join(ports))

So the problem is that we split the portlist property on whitespace and then put it into a set, which is an unordered data structure. So if variants were separate words, they would become disassociated from their ports. We want it to be a set so that we can update it with the list of ports modified by the commit. (Is there ever a time where we have a build that deals with both a forced list of ports and a list of ports from a commit? How would that happen?) I am guessing that Python has ordered data structures that wouldn't have this problem, but that they don't have an update command?

We could change this code so that a portlist specified in the more natural name +var1-var2 -var3 form is converted to your name@+var1-var2-var3 syntax.

I guess you are passing this name@+var1-var2-var3-style string on to the port builder's portname property, from which it gets sent to mpbb as a single argument. I had always assumed that when we implemented variant support in the buildbot, the variant names would be a separate build property from the port name.

and also modify processing of the mpbb list-subport output of the buildbot to treat lines as port specs (since it currently just splits at any whitespace).

I suppose so. We currently process the mpbb list-subports output with:

        subports = [x.strip() for x in stdout.splitlines()]
        return {'subportlist': ' '.join(sorted(subports))}

So the only problem there is that we call sorted; if we didn't do that, variants as separate words wouldn't get separated from their ports.

Of course this then introduces the problem of making this change in a backwards-compatible manner with the buildbot configuration, because we cannot change both at the same time.

I'm not clear what the problem is here. Nobody is specifying variants to the buildbot right now because it doesn't have that feature. Anybody specifying a port list is giving a whitespace-separated list of port names. And that would still be valid in the future. Can't we change the buildbot config and mpbb in either order without breaking anything?

[neverpanic]

I see you've done a lot of work on this already so if you don't want to change it

It wasn't that much work.

So the problem is that we split the portlist property on whitespace and then put it into a set, which is an unordered data structure. So if variants were separate words, they would become disassociated from their ports.

No, the problem is that there is no difference between the separators between a port and its variants, and separate ports. This is unnecessarily hard to parse because loops of the form for x in args are no longer sufficient to loop over ports. We could also switch to a comma-separated portlist and use spaces to separate variants from portnames.

We want it to be a set so that we can update it with the list of ports modified by the commit. (Is there ever a time where we have a build that deals with both a forced list of ports and a list of ports from a commit? How would that happen?)

I don't think that ever happens. Not sure why we wrote the code this way.

I guess you are passing this name@+var1-var2-var3-style string on to the port builder's portname property, from which it gets sent to mpbb as a single argument. I had always assumed that when we implemented variant support in the buildbot, the variant names would be a separate build property from the port name.

It could be passed as a separate argument for a builder that only accepts a single port name, but then again we would have to change the buildbot configuration to achieve variant support. I do not want to cause more work on deploying changes than is necessary.

Additionally, the method of only having a single field for variants doesn't fit to builders that support a portlist argument, if you want to specify separate variants.

So the only problem there is that we call sorted; if we didn't do that, variants as separate words wouldn't get separated from their ports.

But there would still not be an easy way to separate port names from variant specifications, as explained above. I really don't want to add lengthy parsing logic in bash but just use a for i in arguments loop.

I'm not clear what the problem is here. Nobody is specifying variants to the buildbot right now because it doesn't have that feature. Anybody specifying a port list is giving a whitespace-separated list of port names. And that would still be valid in the future. Can't we change the buildbot config and mpbb in either order without breaking anything?

Yes, we can. I was just trying to do a fast iteration to enable variant building that could be improved on later. I think less dependencies between things are a good thing.

[ryandesign]

I agree that the web form should not have a separate variants field; we should specify ports and variants in a single field, like we do on the command line when calling port.

You're right, we can do it this way for now and refine it later.

[neverpanic]

Unfortunately the revert I did at the bottom of the commit stack is not correct, so I'll have to find a different solution for that :/

[neverpanic]

I think macports/macports-ports@45c59fe may actually fix these problems so that the revert won't be necessary. I'll test and report back.

[mojca]

The code currently conflicts with 97f4db4.

[mojca]

@neverpanic or @jmroot: does anyone have time to fix the conflict?

[neverpanic]

I've looked at it, but unfortunately it's not very simple to fix. :/

[mojca]

What should we do?