freebsd / poudriere

Port/Package build and test system
https://github.com/freebsd/poudriere/wiki
BSD 2-Clause "Simplified" License
379 stars 161 forks source link

Add subpackages support #1061

Closed pizzamig closed 8 months ago

pizzamig commented 1 year ago

This patch add the support for subpackages in poudriere.

The subpackage feature is introduces by the revision https://reviews.freebsd.org/D40549 Two test ports have been added in the revision https://reviews.freebsd.org/D40550 and they have been used to develop this patch.

I'm looking for comments and suggestions to not break existing conventions and to keep the code as simple as possible.

pizzamig commented 9 months ago

Wouldn't it be easier to make originspec_decode accept an optional 4th argument ? Adding a new function with '2' suffix makes it hard to read the code and also you need to modify all the code everywhere to use it (which is what you do in the next commits). I agree that I didn't fully re-read all this code so I don't know how easily it would be feasible.

I guess you are looking at an older version of the PR. I've got rid of the version 2of decode/encode already and made 4 argument mandatory everywhere (also in tests, that I broke diligently). I've introduced the 2 functions to track where it was needed, moved every use of encode/decode to encode2/decode2 and then renamed the functions back to encode/decode. I used this complicated process because it's sh, not a true programming language, so I couldn't detect signature mismatches at "compile" time.

I've just noticed that I still have to fix the decode function in json.awk

evadot commented 9 months ago

Wouldn't it be easier to make originspec_decode accept an optional 4th argument ? Adding a new function with '2' suffix makes it hard to read the code and also you need to modify all the code everywhere to use it (which is what you do in the next commits). I agree that I didn't fully re-read all this code so I don't know how easily it would be feasible.

I guess you are looking at an older version of the PR. I've got rid of the version 2of decode/encode already and made 4 argument mandatory everywhere (also in tests, that I broke diligently). I've introduced the 2 functions to track where it was needed, moved every use of encode/decode to encode2/decode2 and then renamed the functions back to encode/decode. I used this complicated process because it's sh, not a true programming language, so I couldn't detect signature mismatches at "compile" time.

I've just noticed that I still have to fix the decode function in json.awk

I've looked at this PR but commit per commit, which github doesn't really handle for reviews it seems ...

evadot commented 9 months ago

Ok I understand what you did, honestly it's way easier for futur reader to : 1/ modify originspec_decode to accept an optional fourth arg 2/ Add it to the arg when we call originspec_decode when you need it.

A bonus point would be to also make flavor an optional arg and everywhere we call originspec with '' for flavor remove this.

pizzamig commented 9 months ago

I was confused by eargs and I thought that optional arguments would have been a bad idea.

I've found example with optional arguments. The implementation has been pushed already

evadot commented 9 months ago

Hi Luca, I think that the overall diff looks good to me, but can you squash a few commits together to have a more readable commit log and git blame in the future ? Especially all the ones about origin_spec.

Cheers,

bapt commented 9 months ago

From my prespective it looks good, can you squash your changes either into a single commit or a serie of "clean" commits?

pizzamig commented 9 months ago

Hi both. Yes, my goal has always been to squash into one commit, not to merge with this messy history. However, I don't have rights to squash and merge. Project maintainers can hit the merge button selecting squash and merge

igalic commented 9 months ago

@pizzamig you can squash it, and give it a bhyve commit message, and people with merge access can merge it

bapt commented 9 months ago

what you can do is squash and rebuild the history locally then if you push --force to your branch we will have here a history that make sense with your commit log, I won't expect the whole thing to come as a single commit, but now if you tell me it should come as one single commit, I will squash & merge.

bapt commented 8 months ago

merged another way, thank you