Re: [QGIS-Developer] QGIS repository management
Interesting indeed. Maybe its because of the the multistage build when it comes to the non working job. That's the only big difference I see between the 2. > Yes, pretty much. > Interesting is, that for the other job based on fedora 38 the cache works, > see e.g. > working: > https://github.com/qgis/QGIS/actions/runs/6560633128/job/17818675001#step:7:117 > not working: > https://github.com/qgis/QGIS/actions/runs/6560633128/job/17818674503#step:7:182 > > Matthias > > On Wed, Oct 18, 2023 at 4:36 PM Julien Cabieces > wrote: > > > It looks like this is a docker container, this could possibly be cached in > the ghcr? > > It's part of the qgis3-qt5-build-deps.dockerfile [0] which is built with a > cache github action [1]. But looking at the log, it doesn't look like cache > is at work, everything seems to be downloaded and built inside the > docker. > > Is it what you are refering to? or is it something else? > > [0] > > https://github.com/qgis/QGIS/blob/master/.docker/qgis3-qt5-build-deps.dockerfile#L165 > [1] > https://github.com/qgis/QGIS/blob/master/.github/workflows/run-tests.yml#L117 > > > On Wed, Oct 18, 2023 at 9:11 AM Julien Cabieces > wrote: > > > > > Is there anything I've missed? > > > > A missing external ressource (OTB, oracle binaries...) that we get with > curl. This one for > > instance recently : > > > https://github.com/qgis/QGIS/actions/runs/6549790513/job/17793674746?pr=54963 > > > > Maybe we could store those ressources somewhere (a github repository for > > instance?) to avoid those errors. > > > > It looks like this is a docker container, this could possibly be cached in > the ghcr? > > -- > > Julien Cabieces > Senior Developer at Oslandia > julien.cabie...@oslandia.com -- Julien Cabieces Senior Developer at Oslandia julien.cabie...@oslandia.com ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
Yes, pretty much. Interesting is, that for the other job based on fedora 38 the cache works, see e.g. working: https://github.com/qgis/QGIS/actions/runs/6560633128/job/17818675001#step:7:117 not working: https://github.com/qgis/QGIS/actions/runs/6560633128/job/17818674503#step:7:182 Matthias On Wed, Oct 18, 2023 at 4:36 PM Julien Cabieces < julien.cabie...@oslandia.com> wrote: > > > It looks like this is a docker container, this could possibly be cached > in the ghcr? > > It's part of the qgis3-qt5-build-deps.dockerfile [0] which is built with a > cache github action [1]. But looking at the log, it doesn't look like cache > is at work, everything seems to be downloaded and built inside the > docker. > > Is it what you are refering to? or is it something else? > > [0] > > https://github.com/qgis/QGIS/blob/master/.docker/qgis3-qt5-build-deps.dockerfile#L165 > [1] > https://github.com/qgis/QGIS/blob/master/.github/workflows/run-tests.yml#L117 > > > On Wed, Oct 18, 2023 at 9:11 AM Julien Cabieces < > julien.cabie...@oslandia.com> wrote: > > > > > Is there anything I've missed? > > > > A missing external ressource (OTB, oracle binaries...) that we get with > curl. This one for > > instance recently : > > > https://github.com/qgis/QGIS/actions/runs/6549790513/job/17793674746?pr=54963 > > > > Maybe we could store those ressources somewhere (a github repository for > > instance?) to avoid those errors. > > > > It looks like this is a docker container, this could possibly be cached > in the ghcr? > > > -- > > Julien Cabieces > Senior Developer at Oslandia > julien.cabie...@oslandia.com > ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
> It looks like this is a docker container, this could possibly be cached in > the ghcr? It's part of the qgis3-qt5-build-deps.dockerfile [0] which is built with a cache github action [1]. But looking at the log, it doesn't look like cache is at work, everything seems to be downloaded and built inside the docker. Is it what you are refering to? or is it something else? [0] https://github.com/qgis/QGIS/blob/master/.docker/qgis3-qt5-build-deps.dockerfile#L165 [1] https://github.com/qgis/QGIS/blob/master/.github/workflows/run-tests.yml#L117 > On Wed, Oct 18, 2023 at 9:11 AM Julien Cabieces > wrote: > > > Is there anything I've missed? > > A missing external ressource (OTB, oracle binaries...) that we get with > curl. This one for > instance recently : > https://github.com/qgis/QGIS/actions/runs/6549790513/job/17793674746?pr=54963 > > Maybe we could store those ressources somewhere (a github repository for > instance?) to avoid those errors. > > It looks like this is a docker container, this could possibly be cached in > the ghcr? -- Julien Cabieces Senior Developer at Oslandia julien.cabie...@oslandia.com ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
On Wed, Oct 18, 2023 at 9:11 AM Julien Cabieces < julien.cabie...@oslandia.com> wrote: > > > Is there anything I've missed? > > > A missing external ressource (OTB, oracle binaries...) that we get with > curl. This one for > instance recently : > > https://github.com/qgis/QGIS/actions/runs/6549790513/job/17793674746?pr=54963 > > Maybe we could store those ressources somewhere (a github repository for > instance?) to avoid those errors. > It looks like this is a docker container, this could possibly be cached in the ghcr? ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
> Is there anything I've missed? A missing external ressource (OTB, oracle binaries...) that we get with curl. This one for instance recently : https://github.com/qgis/QGIS/actions/runs/6549790513/job/17793674746?pr=54963 Maybe we could store those ressources somewhere (a github repository for instance?) to avoid those errors. > network access manager test: depends on the badssl external website, > which goes down occasionally. We could potentially run this site > ourselves as part of the CI setup, but that's trading the dependence > on an external service for added CI maintenance burden on ourselves. > That's what we do for webdav and minio tests : https://github.com/qgis/QGIS/blob/master/.docker/docker-compose-testing.yml#L14 I don't know much about bad ssl returned response but is it that much complicated to configure ? Regards, Julien > On Tue, Oct 17, 2023 at 9:19 PM Nyall Dawson via QGIS-Developer > wrote: > > > > > IMHO, the main issue here is that the CI is too often broken for no > > reasons related to the PR content. And most of the time recently, it's > > because of one of the mingw jobs. > > I suggest we move this conversation into more practical areas and > focus instead on the actual CI issues. > > In my experience there's a couple of main issues here: > > 1. The mingw build fails often with the error: > > cd /__w/QGIS/QGIS/build_mingw64/resources && /usr/bin/cmake -E copy > /__w/QGIS/QGIS/resources/srs6.db > /__w/QGIS/QGIS/build_mingw64/resources/srs.db > make[2]: write error: stdout > > It always succeeds if you restart the job though. I don't know what's > causing this... a disk space issue? (but then why would it succeed on > the following attempt?) > > mingw runs on ubuntu-latest within a fedora container. ubuntu images come > with plenty of bloat preinstalled that can be easily deleted. > cf. > https://github.com/easimon/maximize-build-space/blob/master/action.yml#L114-L130 > I just don't know how to do that if a "container" is specified as the delete > action will need to happen on the base system. > > > 2. The msys2 build can take HOURS (5+ in some cases). The qt 5/ qt6 > builds also see occasional extremely slow builds. > > I think the root cause of this is that we've exhausted all the > available space allowed for caching objects on github. All the builds > are trying to cache things to speed up the workflows, but the > different caches are randomly getting deleted because together they've > well exceeded the total allowed size. The solutions I could see here > are: > > - approaching github to ask nicely for more space > - dropping one or more workflows. Maybe we could combine the msys2 and > mingw workflow into one to cut out one workflow. But that's blocked > because the msys2 workflow currently has a lot of things disabled > (including python), so it's currently not a suitable replacement for > the mingw builds which give end users an easy way to test PRs on > windows. > > What is the advantage of the msys2 workflow? > It does "some build on windows" but IIUC does not replicate the actual > windows build using OSGEO4W. Is there something else that it brings to > the mix? > > > 3. Random test failures. Main culprits are: > - ept provider test (but I **think** I've fixed this with > improvements/fixes made over the 3.34 bug fixing period). At least, I > don't recall seeing this in the last week, when it used to be a daily > random fail. > - raster file writer test: randomly fails for an unknown reason. The > test passes valgrind/asan fine, so I suspect the random failures are > caused by some interplay with other tests modifying data in the test > data folder. More work is needed. > - raster analysis utils test: same as above. > - network access manager test: depends on the badssl external website, > which goes down occasionally. We could potentially run this site > ourselves as part of the CI setup, but that's trading the dependence > on an external service for added CI maintenance burden on ourselves. > > 4. Docker/ubuntu repository fails. This happens occasionally -- the > external repos will have transient connectivity issues. Not much we > can do here except manually re-running the jobs. > > Is there anything I've missed? > > There is also the occasional "QGIS is not compatible with the bleeding edge > [sip, name your dependency] version that [rolling distro] has > decided to start shipping today". > > Matthias > > > Nyall > > > > > Like proposed by Matthias, I would be very much in favor in modifying > > our CI jobs to rely on non-rolling release distribution. > > > > Regards, > > Julien > > > > > > > In my experience, the peer reviews have proven to be an effective tool > to improve the code quality. > > > I think it can be explained with a four eyes principle. The first two > eyes are involved in writing the code, the second pair of eyes that > validates > > > needs to be a
Re: [QGIS-Developer] QGIS repository management
On Tue, Oct 17, 2023 at 9:19 PM Nyall Dawson via QGIS-Developer < qgis-developer@lists.osgeo.org> wrote: > > > > IMHO, the main issue here is that the CI is too often broken for no > > reasons related to the PR content. And most of the time recently, it's > > because of one of the mingw jobs. > > I suggest we move this conversation into more practical areas and > focus instead on the actual CI issues. > > In my experience there's a couple of main issues here: > > 1. The mingw build fails often with the error: > > cd /__w/QGIS/QGIS/build_mingw64/resources && /usr/bin/cmake -E copy > /__w/QGIS/QGIS/resources/srs6.db > /__w/QGIS/QGIS/build_mingw64/resources/srs.db > make[2]: write error: stdout > > It always succeeds if you restart the job though. I don't know what's > causing this... a disk space issue? (but then why would it succeed on > the following attempt?) > mingw runs on ubuntu-latest within a fedora container. ubuntu images come with plenty of bloat preinstalled that can be easily deleted. cf. https://github.com/easimon/maximize-build-space/blob/master/action.yml#L114-L130 I just don't know how to do that if a "container" is specified as the delete action will need to happen on the base system. > > 2. The msys2 build can take HOURS (5+ in some cases). The qt 5/ qt6 > builds also see occasional extremely slow builds. > > I think the root cause of this is that we've exhausted all the > available space allowed for caching objects on github. All the builds > are trying to cache things to speed up the workflows, but the > different caches are randomly getting deleted because together they've > well exceeded the total allowed size. The solutions I could see here > are: > > - approaching github to ask nicely for more space > - dropping one or more workflows. Maybe we could combine the msys2 and > mingw workflow into one to cut out one workflow. But that's blocked > because the msys2 workflow currently has a lot of things disabled > (including python), so it's currently not a suitable replacement for > the mingw builds which give end users an easy way to test PRs on > windows. > What is the advantage of the msys2 workflow? It does "some build on windows" but IIUC does not replicate the actual windows build using OSGEO4W. Is there something else that it brings to the mix? > > 3. Random test failures. Main culprits are: > - ept provider test (but I **think** I've fixed this with > improvements/fixes made over the 3.34 bug fixing period). At least, I > don't recall seeing this in the last week, when it used to be a daily > random fail. > - raster file writer test: randomly fails for an unknown reason. The > test passes valgrind/asan fine, so I suspect the random failures are > caused by some interplay with other tests modifying data in the test > data folder. More work is needed. > - raster analysis utils test: same as above. > - network access manager test: depends on the badssl external website, > which goes down occasionally. We could potentially run this site > ourselves as part of the CI setup, but that's trading the dependence > on an external service for added CI maintenance burden on ourselves. > > 4. Docker/ubuntu repository fails. This happens occasionally -- the > external repos will have transient connectivity issues. Not much we > can do here except manually re-running the jobs. > > Is there anything I've missed? > There is also the occasional "QGIS is not compatible with the bleeding edge [sip, name your dependency] version that [rolling distro] has decided to start shipping today". Matthias > > Nyall > > > > > > > > > > > > > Like proposed by Matthias, I would be very much in favor in modifying > > our CI jobs to rely on non-rolling release distribution. > > > > Regards, > > Julien > > > > > > > In my experience, the peer reviews have proven to be an effective tool > to improve the code quality. > > > I think it can be explained with a four eyes principle. The first two > eyes are involved in writing the code, the second pair of eyes that > validates > > > needs to be a trusted pair. > > > For a code base of the maturity of QGIS, this seems to me to be > justified and reasonable. > > > > > > As for who can do a review, a potential conflict of interest has been > raised. This arises most noticeable within a single company, but can also > > > very well be the case whenever a collaboration occurs between multiple > individuals or if a well-connected client is involved. In my experience, in > > > most cases people will first ask the person that they trust to be most > qualified to review a specific area of the code, regardless the organisation > > > they work for. In some cases, this may be your colleagues. What I and > others have done in the past in such cases, is to leave an additional review > > > window of a couple of days before merging to give everyone the > opportunity to jump in if they wanted to, just to be sure to play it fair. > In my > > > experience, this worked quite
Re: [QGIS-Developer] QGIS repository management
On Wed, Oct 18, 2023 at 08:18:46AM +1300, Nyall Dawson via QGIS-Developer wrote: > > > > IMHO, the main issue here is that the CI is too often broken for no > > reasons related to the PR content. And most of the time recently, it's > > because of one of the mingw jobs. > > I suggest we move this conversation into more practical areas and > focus instead on the actual CI issues. CI was only 50% of the issue I raised, the other 50% was government issues, which practically is acted upon by: 1. Make roles clearly documented: - who can change github configuration in case of problems - who is allowed to push to which branches - who can apply labels to issues/PRs - how the above privileges are granted and revoked > In my experience there's a couple of main issues here: > > 1. The mingw build fails often with the error: > 2. The msys2 build can take HOURS (5+ in some cases) > 3. Random test failures. Main culprits are: > 4. Docker/ubuntu repository fails > > Is there anything I've missed? Laurențiu (grayshade in Matrix) noticed the Ubuntu-22.04 matrix build is far slower than the Fedora 38 (one), I didn't dig that up. Another thing I hit was layout failures in CI that could have been prevented by having those tests run locally by pre-commit hooks, like with this 3-days old and green PR: https://github.com/qgis/QGIS/pull/54936 Each PR triggers 28 workflows, and I looks like a failure in a layout workflow does not stop the others which contend resources with other PRs. Today I've seen up to 44 workflows running in parallel, which slow down each one of them. The more tests developers can run locally, the lighter we are on the central infrastructure. In general, I think it's easier if each of these issues we find is filed as a ticket and labeled with "testsuite" [1] and/or "CI/GitHub-Actions" [2] as appropriate. [1] https://github.com/qgis/QGIS/issues?q=is%3Aopen+is%3Aissue+label%3Atestsuite [2] https://github.com/qgis/QGIS/issues?q=is%3Aopen+is%3Aissue+label%3ACI%2FGitHub-Actions --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
> > IMHO, the main issue here is that the CI is too often broken for no > reasons related to the PR content. And most of the time recently, it's > because of one of the mingw jobs. I suggest we move this conversation into more practical areas and focus instead on the actual CI issues. In my experience there's a couple of main issues here: 1. The mingw build fails often with the error: cd /__w/QGIS/QGIS/build_mingw64/resources && /usr/bin/cmake -E copy /__w/QGIS/QGIS/resources/srs6.db /__w/QGIS/QGIS/build_mingw64/resources/srs.db make[2]: write error: stdout It always succeeds if you restart the job though. I don't know what's causing this... a disk space issue? (but then why would it succeed on the following attempt?) 2. The msys2 build can take HOURS (5+ in some cases). The qt 5/ qt6 builds also see occasional extremely slow builds. I think the root cause of this is that we've exhausted all the available space allowed for caching objects on github. All the builds are trying to cache things to speed up the workflows, but the different caches are randomly getting deleted because together they've well exceeded the total allowed size. The solutions I could see here are: - approaching github to ask nicely for more space - dropping one or more workflows. Maybe we could combine the msys2 and mingw workflow into one to cut out one workflow. But that's blocked because the msys2 workflow currently has a lot of things disabled (including python), so it's currently not a suitable replacement for the mingw builds which give end users an easy way to test PRs on windows. 3. Random test failures. Main culprits are: - ept provider test (but I **think** I've fixed this with improvements/fixes made over the 3.34 bug fixing period). At least, I don't recall seeing this in the last week, when it used to be a daily random fail. - raster file writer test: randomly fails for an unknown reason. The test passes valgrind/asan fine, so I suspect the random failures are caused by some interplay with other tests modifying data in the test data folder. More work is needed. - raster analysis utils test: same as above. - network access manager test: depends on the badssl external website, which goes down occasionally. We could potentially run this site ourselves as part of the CI setup, but that's trading the dependence on an external service for added CI maintenance burden on ourselves. 4. Docker/ubuntu repository fails. This happens occasionally -- the external repos will have transient connectivity issues. Not much we can do here except manually re-running the jobs. Is there anything I've missed? Nyall > > Like proposed by Matthias, I would be very much in favor in modifying > our CI jobs to rely on non-rolling release distribution. > > Regards, > Julien > > > > In my experience, the peer reviews have proven to be an effective tool to > > improve the code quality. > > I think it can be explained with a four eyes principle. The first two eyes > > are involved in writing the code, the second pair of eyes that validates > > needs to be a trusted pair. > > For a code base of the maturity of QGIS, this seems to me to be justified > > and reasonable. > > > > As for who can do a review, a potential conflict of interest has been > > raised. This arises most noticeable within a single company, but can also > > very well be the case whenever a collaboration occurs between multiple > > individuals or if a well-connected client is involved. In my experience, in > > most cases people will first ask the person that they trust to be most > > qualified to review a specific area of the code, regardless the organisation > > they work for. In some cases, this may be your colleagues. What I and > > others have done in the past in such cases, is to leave an additional review > > window of a couple of days before merging to give everyone the opportunity > > to jump in if they wanted to, just to be sure to play it fair. In my > > experience, this worked quite nicely, but I am also open to other > > approaches. > > > > As for failing tests, I think one possibly low hanging fruit would be to > > switch all CI workflows to be based on non-rolling distributions. And we > > could potentially also be a bit more strict with disabling flaky tests or > > making them optional. > > > > Best wishes > > Matthias > > > > On Tue, Oct 17, 2023 at 9:47 AM Sandro Santilli via QGIS-Developer > > wrote: > > > > On Tue, Oct 17, 2023 at 03:08:34PM +1300, Nyall Dawson via QGIS-Developer > > wrote: > > > On Tue, 17 Oct 2023 at 03:41, Sandro Santilli wrote: > > > > > > > > On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via > > QGIS-Developer wrote: > > > > > > > > > If you flip the situation, you'll see that yes, you do have trust! > > > > > > > > > > - a complete stranger CANNOT approve their own changes > > > > > - a complete stranger CANNOT approve other stranger's changes > > > > > - a complete stranger CANNOT approve an
Re: [QGIS-Developer] QGIS repository management
Hi, Even if it's sometime painful to wait for a review, I completely suscribe to the actual processus of review by another peer for the reasons well explained by others. IMHO, the main issue here is that the CI is too often broken for no reasons related to the PR content. And most of the time recently, it's because of one of the mingw jobs. Like proposed by Matthias, I would be very much in favor in modifying our CI jobs to rely on non-rolling release distribution. Regards, Julien > In my experience, the peer reviews have proven to be an effective tool to > improve the code quality. > I think it can be explained with a four eyes principle. The first two eyes > are involved in writing the code, the second pair of eyes that validates > needs to be a trusted pair. > For a code base of the maturity of QGIS, this seems to me to be justified and > reasonable. > > As for who can do a review, a potential conflict of interest has been raised. > This arises most noticeable within a single company, but can also > very well be the case whenever a collaboration occurs between multiple > individuals or if a well-connected client is involved. In my experience, in > most cases people will first ask the person that they trust to be most > qualified to review a specific area of the code, regardless the organisation > they work for. In some cases, this may be your colleagues. What I and others > have done in the past in such cases, is to leave an additional review > window of a couple of days before merging to give everyone the opportunity to > jump in if they wanted to, just to be sure to play it fair. In my > experience, this worked quite nicely, but I am also open to other approaches. > > As for failing tests, I think one possibly low hanging fruit would be to > switch all CI workflows to be based on non-rolling distributions. And we > could potentially also be a bit more strict with disabling flaky tests or > making them optional. > > Best wishes > Matthias > > On Tue, Oct 17, 2023 at 9:47 AM Sandro Santilli via QGIS-Developer > wrote: > > On Tue, Oct 17, 2023 at 03:08:34PM +1300, Nyall Dawson via QGIS-Developer > wrote: > > On Tue, 17 Oct 2023 at 03:41, Sandro Santilli wrote: > > > > > > On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via > QGIS-Developer wrote: > > > > > > > If you flip the situation, you'll see that yes, you do have trust! > > > > > > > > - a complete stranger CANNOT approve their own changes > > > > - a complete stranger CANNOT approve other stranger's changes > > > > - a complete stranger CANNOT approve an approved member's changes > > > > > > > > vs > > > > > > > > - an approved member CANNOT approve their own changes > > > > - an approved member CAN approve a complete stranger's changes > > > > - an approved member CAN approve a another approved member's changes > > > > > > That's partial trust. I'm trusted to be able to judge someone else's > > > work but not to judge my own work ! > > > > Ok, it's partial trust. What's wrong with that? > > I feel I'm not explaining myself correctly. > I do believe in peer review, and I'm happy for others to look at the > changes I propose. I'd love everyone to look at them! > > The word "partial" was not appropriate, probably, it's more about > "inconsistency". > > > In short: I'm more than OK with only partial trusting everyone. We ALL > > make mistakes, so let's stick with the processes we have which reduce > > the risk of these mistakes hitting end users.. > > I know I make mistakes, but I'm able to merge changes proposed by a > random contributor, where my own review is enough. > > I'm FULLY trusted in that case, and this is conflicting with not > being so when it's me proposing the changes. > > Why should I be able to merge in that case then ? Is it because > 2 people review the code ? Me and the proponent ? Then why the > random contributor review would not count on MY prs ? > > I hope you see the discrepancy here. > > --strk; > >Libre GIS consultant/developer >https://strk.kbt.io/services.html > ___ > QGIS-Developer mailing list > QGIS-Developer@lists.osgeo.org > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer > > ___ > QGIS-Developer mailing list > QGIS-Developer@lists.osgeo.org > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer -- Julien Cabieces Senior Developer at Oslandia julien.cabie...@oslandia.com ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
In my experience, the peer reviews have proven to be an effective tool to improve the code quality. I think it can be explained with a four eyes principle. The first two eyes are involved in writing the code, the second pair of eyes that validates needs to be a trusted pair. For a code base of the maturity of QGIS, this seems to me to be justified and reasonable. As for who can do a review, a potential conflict of interest has been raised. This arises most noticeable within a single company, but can also very well be the case whenever a collaboration occurs between multiple individuals or if a well-connected client is involved. In my experience, in most cases people will first ask the person that they trust to be most qualified to review a specific area of the code, regardless the organisation they work for. In some cases, this may be your colleagues. What I and others have done in the past in such cases, is to leave an additional review window of a couple of days before merging to give everyone the opportunity to jump in if they wanted to, just to be sure to play it fair. In my experience, this worked quite nicely, but I am also open to other approaches. As for failing tests, I think one possibly low hanging fruit would be to switch all CI workflows to be based on non-rolling distributions. And we could potentially also be a bit more strict with disabling flaky tests or making them optional. Best wishes Matthias On Tue, Oct 17, 2023 at 9:47 AM Sandro Santilli via QGIS-Developer < qgis-developer@lists.osgeo.org> wrote: > On Tue, Oct 17, 2023 at 03:08:34PM +1300, Nyall Dawson via QGIS-Developer > wrote: > > On Tue, 17 Oct 2023 at 03:41, Sandro Santilli wrote: > > > > > > On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via > QGIS-Developer wrote: > > > > > > > If you flip the situation, you'll see that yes, you do have trust! > > > > > > > > - a complete stranger CANNOT approve their own changes > > > > - a complete stranger CANNOT approve other stranger's changes > > > > - a complete stranger CANNOT approve an approved member's changes > > > > > > > > vs > > > > > > > > - an approved member CANNOT approve their own changes > > > > - an approved member CAN approve a complete stranger's changes > > > > - an approved member CAN approve a another approved member's changes > > > > > > That's partial trust. I'm trusted to be able to judge someone else's > > > work but not to judge my own work ! > > > > Ok, it's partial trust. What's wrong with that? > > I feel I'm not explaining myself correctly. > I do believe in peer review, and I'm happy for others to look at the > changes I propose. I'd love everyone to look at them! > > The word "partial" was not appropriate, probably, it's more about > "inconsistency". > > > In short: I'm more than OK with only partial trusting everyone. We ALL > > make mistakes, so let's stick with the processes we have which reduce > > the risk of these mistakes hitting end users.. > > I know I make mistakes, but I'm able to merge changes proposed by a > random contributor, where my own review is enough. > > I'm FULLY trusted in that case, and this is conflicting with not > being so when it's me proposing the changes. > > Why should I be able to merge in that case then ? Is it because > 2 people review the code ? Me and the proponent ? Then why the > random contributor review would not count on MY prs ? > > I hope you see the discrepancy here. > > --strk; > > Libre GIS consultant/developer > https://strk.kbt.io/services.html > ___ > QGIS-Developer mailing list > QGIS-Developer@lists.osgeo.org > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer > ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
On Tue, Oct 17, 2023 at 03:08:34PM +1300, Nyall Dawson via QGIS-Developer wrote: > On Tue, 17 Oct 2023 at 03:41, Sandro Santilli wrote: > > > > On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via QGIS-Developer > > wrote: > > > > > If you flip the situation, you'll see that yes, you do have trust! > > > > > > - a complete stranger CANNOT approve their own changes > > > - a complete stranger CANNOT approve other stranger's changes > > > - a complete stranger CANNOT approve an approved member's changes > > > > > > vs > > > > > > - an approved member CANNOT approve their own changes > > > - an approved member CAN approve a complete stranger's changes > > > - an approved member CAN approve a another approved member's changes > > > > That's partial trust. I'm trusted to be able to judge someone else's > > work but not to judge my own work ! > > Ok, it's partial trust. What's wrong with that? I feel I'm not explaining myself correctly. I do believe in peer review, and I'm happy for others to look at the changes I propose. I'd love everyone to look at them! The word "partial" was not appropriate, probably, it's more about "inconsistency". > In short: I'm more than OK with only partial trusting everyone. We ALL > make mistakes, so let's stick with the processes we have which reduce > the risk of these mistakes hitting end users.. I know I make mistakes, but I'm able to merge changes proposed by a random contributor, where my own review is enough. I'm FULLY trusted in that case, and this is conflicting with not being so when it's me proposing the changes. Why should I be able to merge in that case then ? Is it because 2 people review the code ? Me and the proponent ? Then why the random contributor review would not count on MY prs ? I hope you see the discrepancy here. --strk; Libre GIS consultant/developer https://strk.kbt.io/services.html ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
On Tue, 17 Oct 2023 at 03:41, Sandro Santilli wrote: > > On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via QGIS-Developer > wrote: > > > If you flip the situation, you'll see that yes, you do have trust! > > > > - a complete stranger CANNOT approve their own changes > > - a complete stranger CANNOT approve other stranger's changes > > - a complete stranger CANNOT approve an approved member's changes > > > > vs > > > > - an approved member CANNOT approve their own changes > > - an approved member CAN approve a complete stranger's changes > > - an approved member CAN approve a another approved member's changes > > That's partial trust. I'm trusted to be able to judge someone else's > work but not to judge my own work ! Ok, it's partial trust. What's wrong with that? I wouldn't trust myself not to make accidental mistakes, or do something inefficient, or that I'm not duplicating code which is present elsewhere in QGIS. A second set of eyes over a pull request definitely reduces that risk. QGIS is MASSIVE, and none of us can keep a complete picture of the impact of code changes in our heads. Some real world examples, picked at semi-random (because they are recent): https://github.com/qgis/QGIS/pull/54941 : Submitted by a core developer. I reviewed, and it looked good to me. But then a third (!) set of eyes over the code has revealed a massive potential performance regression caused by the change (thanks Even!). https://github.com/qgis/QGIS/pull/54670: Submitted by a very experienced core developer. Code looks good at first pass, no visible bugs. But it's introducing a partial duplication of another utility function implemented elsewhere and exhaustively tested. The review identified that and as a result we avoided introducing an unnecessary, non trivial duplication of code. That's two examples. I'd very conservatively estimate that 1 in 10 approved PRs submitted by core committers have at least one set of changes suggested and implemented. Let's (pessimistically!) write half of those off as non-bugs, such as optimisation suggestions or changes relating to documentation/comments/code style. That's still **AT BEST** 1 in 20 pull requests submitted by core committers which needed fixes. In short: I'm more than OK with only partial trusting everyone. We ALL make mistakes, so let's stick with the processes we have which reduce the risk of these mistakes hitting end users.. Nyall > > Beside the same-company reviews things, consider I could always ask a > friend to file PRs for the sole purpose of me being able to merge them > in (I cannot merge my own...). The policy is flawed to me. > > --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
[Intranet logo] I'm far from a regular contributor to QGIS and thus not affected by this issue, but I see this review process the same way I see how peer-reviewed research papers get published (but a bit less stringent). In research, your manuscript is sent to 3 or 4 reviewers that will give comments, then you make corrections and resend the manuscript for a final review before getting published. This workflow is valid for every researcher that want to publish in a peer-reviewed journal, even for those that are also reviewer. It can be slow and frustrating at times, but having an other set of eyes on your work usually result in a better overall paper. Software development is different than peer-reviewed research publication but getting inspiration by this centuries old process is certainly appropriate. Going back to QGIS, I see the value in having the PR of a reviewer reviewed by an other reviewer in this second fresh set of eyes that can improve on an already fine job. Every reviewer can be trusted for their expertise and judgement... but needing someone else to review your code shouldn't be seen as distrustful. As for the conflict of interest if the reviewer works for the same company, in the academia world, the reviewer would be in conflict of interest (real or not) and would need to withdraw (the equivalent of a company being the same department of a research institute or a close collaborator). Considering that in academia this constrain has a lot to do with avoiding an idea being pushed without criticism, it might be overkill in software development where the ideas/orientations are not decided on a PR basis, but more at the PSC and with the general consensus between the devs (that's how I see it, I might be wrong). Jean-François Bourdon, ing.f. Analyste en télédétection Direction des inventaires forestiers Ministère des Ressources naturelles et des Forêts 5700, 4e Avenue Ouest, local A-108 Québec (Québec) G1H 6R1 Téléphone : 418 627-8669, poste 704304 jean-francois.bour...@mrnf.gouv.qc.ca mrnf.gouv.qc.ca -Message d'origine- De : QGIS-Developer De la part de Sandro Santilli via QGIS-Developer Envoyé : 16 octobre 2023 10:57 À : Nyall Dawson ; qgis-developer@lists.osgeo.org Objet : Re: [QGIS-Developer] QGIS repository management On Mon, Oct 16, 2023 at 04:41:42PM +0200, Sandro Santilli via QGIS-Developer wrote: > On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via QGIS-Developer > wrote: > > > If you flip the situation, you'll see that yes, you do have trust! > > > > - a complete stranger CANNOT approve their own changes > > - a complete stranger CANNOT approve other stranger's changes > > - a complete stranger CANNOT approve an approved member's changes > > > > vs > > > > - an approved member CANNOT approve their own changes > > - an approved member CAN approve a complete stranger's changes > > - an approved member CAN approve a another approved member's changes > > That's partial trust. I'm trusted to be able to judge someone else's > work but not to judge my own work ! > > Beside the same-company reviews things, consider I could always ask a > friend to file PRs for the sole purpose of me being able to merge them > in (I cannot merge my own...). The policy is flawed to me. My suggestion here is that having given someone "write privileges" status means the community trusts that individual to know the rules and be able to apply them, in a way that does not striclty requires someone else to guard after his/her work. This isn't to say that reviews aren't important and that all of use should always aim at having reviews, but that the judgement abilities of the "writers" should be trusted, and if that trust is not hold anymore the "write privileges" should be revoked, following a documented procedure. --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
On Mon, Oct 16, 2023 at 04:41:42PM +0200, Sandro Santilli via QGIS-Developer wrote: > On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via QGIS-Developer > wrote: > > > If you flip the situation, you'll see that yes, you do have trust! > > > > - a complete stranger CANNOT approve their own changes > > - a complete stranger CANNOT approve other stranger's changes > > - a complete stranger CANNOT approve an approved member's changes > > > > vs > > > > - an approved member CANNOT approve their own changes > > - an approved member CAN approve a complete stranger's changes > > - an approved member CAN approve a another approved member's changes > > That's partial trust. I'm trusted to be able to judge someone else's > work but not to judge my own work ! > > Beside the same-company reviews things, consider I could always ask a > friend to file PRs for the sole purpose of me being able to merge them > in (I cannot merge my own...). The policy is flawed to me. My suggestion here is that having given someone "write privileges" status means the community trusts that individual to know the rules and be able to apply them, in a way that does not striclty requires someone else to guard after his/her work. This isn't to say that reviews aren't important and that all of use should always aim at having reviews, but that the judgement abilities of the "writers" should be trusted, and if that trust is not hold anymore the "write privileges" should be revoked, following a documented procedure. --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via QGIS-Developer wrote: > If you flip the situation, you'll see that yes, you do have trust! > > - a complete stranger CANNOT approve their own changes > - a complete stranger CANNOT approve other stranger's changes > - a complete stranger CANNOT approve an approved member's changes > > vs > > - an approved member CANNOT approve their own changes > - an approved member CAN approve a complete stranger's changes > - an approved member CAN approve a another approved member's changes That's partial trust. I'm trusted to be able to judge someone else's work but not to judge my own work ! Beside the same-company reviews things, consider I could always ask a friend to file PRs for the sole purpose of me being able to merge them in (I cannot merge my own...). The policy is flawed to me. --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
As someone on the outside of the committer set (which is fine!) for qgis, but familiar with the larger issues (and a packager): I'm going to use the word committer. git has renamed things, but the point is of course placing state in the branch that matters in the repo that matters. There's a question about how the rules evolved, and how they change, and that's hard to answer, but it's not really a big issue. The rules not being written down and understandable is a big issue with an easy fix. It is quite easy for someone who knows them to just put them in a HACKING.md or CONTRIBUTING.md if you lean to github vs GNU. Sandro's point about 1 committer approving a stranger's change has some merit, but I think we have to separate: - review to avoid bugs - review to avoid injection of malware Presumably, one would only approve/merge a chagne if it is clean, neat, meets standards, works, etc., and if it were more complicated a committer should ask others/wait/etc. If we're going to insist on CI passing to merge, then it really needs to pass essentially all the time. This means disabling parts of it if they are flaky, or demoting them to a 2nd workflow that is not considered a failure. ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
For completeness: > - an approved member CANNOT approve their own changes > - an approved member CAN approve a complete stranger's changes > - an approved member CAN approve a another approved member's changes A potential issue which has popped up a couple of times is whether it's acceptable for another staff member from the same organisation to approve their colleague's PRs. Is this acceptable or not? Is it a conflict of interest? Does it lean toward lenient reviews and getting a speedy merge? Or is it good practice because the organisation isn't placing any burden on others outside of their organization? 路 Who knows! But definitely work discussing if we are writing up formal policies... Nyall > > I think the misunderstanding is all about naming. If we drop the > confusing "core committer" label and change it to "approved reviewer", > then everything becomes clear. > > (and then we have a separate "super user" group with repo admin level > privileges, which should be kept as SMALL as possibly needed to keep > the repository and CI working, and is unrelated to > code/trust/experience/etc...) > > > Another policy I believe in is that whoever is allowed to apply > > changes to the repository should take on the responsibility of > > fixing bugs introduced by those changes. > > I'm a hesitant +0 to this. I do think we've all got a good track > record of fixing our own mistakes within reasonable expectations. I'd > be concerned that formalising this would put a lot of unreasonable > expectations on developers (where's the limit? If it's found that I > broke some esoteric workflow not covered by unit tests 3 years later, > am I still responsible for fixing that? I'd argue not). > > Nyall > > > > --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
On Mon, 16 Oct 2023 at 09:44, Sandro Santilli wrote: > > On Mon, Oct 16, 2023 at 08:58:30AM +1300, Nyall Dawson wrote: > > On Sat, 14 Oct 2023 at 05:43, Sandro Santilli wrote: > > > > > 2. Allow those with "write access" to self-approve PRs > > > > -1. What's the real motivation here? Why the urgency to get unreviewed code > > into QGIS? > > A recent example of urgency: I broke the pre-commit hook for most > developers, I could have pushed the fix earlier if I didn't have > to wait for a review. That's a fair example, I'll concede that point! I wonder if tagging the commit with "[skip ci]" would still permit the merge request to be merged? It's worth a test. > > The other motivation is not about urgency but about a conflicting policy: > > - I can be the _only_ reviewer approving a complete stranger's > proposed change. > > - I can NOT be the _only_ reviewer approving my proposed change. > > Is trust given to _me_ as a "reviewer" or not ? > It is in the first case, it isn't in the second case. If you flip the situation, you'll see that yes, you do have trust! - a complete stranger CANNOT approve their own changes - a complete stranger CANNOT approve other stranger's changes - a complete stranger CANNOT approve an approved member's changes vs - an approved member CANNOT approve their own changes - an approved member CAN approve a complete stranger's changes - an approved member CAN approve a another approved member's changes I think the misunderstanding is all about naming. If we drop the confusing "core committer" label and change it to "approved reviewer", then everything becomes clear. (and then we have a separate "super user" group with repo admin level privileges, which should be kept as SMALL as possibly needed to keep the repository and CI working, and is unrelated to code/trust/experience/etc...) > Another policy I believe in is that whoever is allowed to apply > changes to the repository should take on the responsibility of > fixing bugs introduced by those changes. I'm a hesitant +0 to this. I do think we've all got a good track record of fixing our own mistakes within reasonable expectations. I'd be concerned that formalising this would put a lot of unreasonable expectations on developers (where's the limit? If it's found that I broke some esoteric workflow not covered by unit tests 3 years later, am I still responsible for fixing that? I'd argue not). Nyall > > --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
On Mon, Oct 16, 2023 at 09:04:38AM +1300, Nyall Dawson wrote: > I have this power too -- and I use it on a daily basis to keep the whole CI > setup flowing (ie restarting workflows in other's PRs, merging approved PRs > when an unrelated workflow failure has blocked a merge, etc). > > I'd like to point out that it's only used to keep the project humming along > for EVERYONE's benefit -- it's not a conspiracy to create a set of "super > committers" with extra power . I know you're not doing any evil, I just find it useful to know who has those powers so I know who to ask to if I need them :) --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
On Mon, Oct 16, 2023 at 08:58:30AM +1300, Nyall Dawson wrote: > On Sat, 14 Oct 2023 at 05:43, Sandro Santilli wrote: > > > 2. Allow those with "write access" to self-approve PRs > > -1. What's the real motivation here? Why the urgency to get unreviewed code > into QGIS? A recent example of urgency: I broke the pre-commit hook for most developers, I could have pushed the fix earlier if I didn't have to wait for a review. The other motivation is not about urgency but about a conflicting policy: - I can be the _only_ reviewer approving a complete stranger's proposed change. - I can NOT be the _only_ reviewer approving my proposed change. Is trust given to _me_ as a "reviewer" or not ? It is in the first case, it isn't in the second case. > Again, I am strongly of the opinion that more exhaustive code > merging policies protects our users and their trust in QGIS. I'm not against policies per-se. For example I'm strongly of the opinion that those who propose changes should run as many tests as possible to verify the effects of those changes, and would give high priority to tasks that aim at making this test runs as smooth as possible. Another policy I believe in is that whoever is allowed to apply changes to the repository should take on the responsibility of fixing bugs introduced by those changes. --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
On Sun, 15 Oct 2023 at 02:54, Sandro Santilli via QGIS-Developer < qgis-developer@lists.osgeo.org> wrote: > > That term "committer", btw, should probably be changed nowadays as > with git it doesnt' make much sense. +1. Maybe we should go with "review approver" to reflect exactly what this privilege means today... > > > The QGIS organization has a budget for PR queue management (triage) > > and a separate entry for reviews (budget is public > > https://www.qgis.org/en/site/getinvolved/governance/finance/index.html) > > , in the past this has been working well while I agree that recently > > PRs have been pending without review for a (too) long time. > > The bugfixing spreadsheet has a column to report reviewer names, > but it looks like I'm the only one filling it at the moment. > You may notice I've also reported reviews from non-committers > ( Laurențiu Nicola, in Cc ). I think everyone is trying to reduce paperwork and that's why that column is unpopulated Nyall ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
> A small group of us (3-4 developers including me) have admin access to
> all QGIS repositories and we can bypass any check and merge all PRs
> without approval.
I have this power too -- and I use it on a daily basis to keep the whole CI
setup flowing (ie restarting workflows in other's PRs, merging approved PRs
when an unrelated workflow failure has blocked a merge, etc).
I'd like to point out that it's only used to keep the project humming along
for EVERYONE's benefit -- it's not a conspiracy to create a set of "super
committers" with extra power .
Nyall
>
> I admit that I have been using these superpowers more and more often
> (IIRC three or four times) in the last year while I have never felt
> the need to use them before, in an ideal world it should not be
> necessary except for the same use cases I have pointed out for the
> direct pushes to master.
>
> The bottom line is that the situation is not perfect and can certainly
> be improved but at the end of the day we are all doing our best to
> keep the process running as smoothly as we can.
>
> Thank you for raising this point, you are absolutely right that it
> should be clearly documented, this has been in our TODO list for a
> long time.
>
> Best regards.
>
>
> On Fri, Oct 13, 2023 at 6:42 PM Sandro Santilli via QGIS-Developer
> wrote:
> >
> > Hello all,
> > today I was finally able to more clearly see the problem that frustrates
> > me everytime a take part to a new QGIS bugfixing drive, and I would like
> > to share it hoping to find a solution togheter.
> >
> > The main problem:
> >
> > - Despite having been granted write access to the QGIS repository in
2012
> > [1], I cannot effectively use that power today
> >
> > It's not just me, I think, but I cannot tell for sure because the
configuration
> > of the infrastructure currently in use (github) is not available for me
to see
> > and the governance page on the official QGIS website does not contain
this
> > information [2]. This being blind of course adds up to my frustration.
> >
> > From experience, I know that the reason why I cannot write to the QGIS
> > repository is because "branch protection" is active (for the master
branch
> > at least) and a set of conditions are required to merge a PR, namely:
> >
> > - All CI tests need to pass.
> >
> > - Someone else (I don't know from which group of people) needs to
> > approve the proposed change.
> >
> > While I do the above condition being a useful indication for "QGIS
> > core developers" to decide whether to accept or not a change request,
> > I find them representing an obstacle way more often than a service,
> > and in particular:
> >
> > 1. CI is often broken for reasons that are independent from the proposed
> >change.
> >
> > 2. An aberration of the "review" condition is that a change proposed by
a
> >contributor and approved by me can be merged but a change proposed by
> >me and approved by the same contributor can not be merged,
effectively
> >giving me ("core QGIS committer") less power than the power of a
random
> >contributor.
> >
> > The rules described above are not found from the governance page [2]
> > so it isn't easy for me to propose changes because I don't have a clear
> > picture of current rules (like, I believe some people in QGIS can
> > self-approve PRs but dunno how to tell who and why).
> >
> > I would personally welcome (and be able to help taking) the following
actions:
> >
> > 1. Clearly document the roles and rules on the website
> >
> > 2. Allow those with "write access" to self-approve PRs
> >
> > 3. Define rules by which "write access" privileges to the repository
> > are revoked
> >
> > Thanks for having read this in full, and I hope to hear your position
> > on the matter. Happy hacking !
> >
> >
> > [1]
https://lists.osgeo.org/pipermail/qgis-developer/2012-October/022715.html
> > [2] https://qgis.org/en/site/getinvolved/governance/governance.html
> >
> > --strk;
> >
> > Libre GIS consultant/developer
> > https://strk.kbt.io/services.html
> > ___
> > QGIS-Developer mailing list
> > QGIS-Developer@lists.osgeo.org
> > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
> > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>
>
>
> --
> Alessandro Pasotti
> QCooperative: www.qcooperative.net
> ItOpen: www.itopen.it
> ___
> QGIS-Developer mailing list
> QGIS-Developer@lists.osgeo.org
> List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
> Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
___
QGIS-Developer mailing list
QGIS-Developer@lists.osgeo.org
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
Hi Sandro,
Thanks for raising this discussion. I'll add my knowledge inline below.
But please keep in mind that all this stuff has just been set over time in
a reactionary/evolutionary process to keep things running smoothly, rather
than something which was designed and formalised by a committee in advance.
There's always room for improvement and revision to match current needs!
On Sat, 14 Oct 2023 at 05:43, Sandro Santilli via QGIS-Developer <
qgis-developer@lists.osgeo.org> wrote:
>
> It's not just me, I think, but I cannot tell for sure because the
configuration
> of the infrastructure currently in use (github) is not available for me
to see
> and the governance page on the official QGIS website does not contain this
> information [2]. This being blind of course adds up to my frustration.
This page **definetly** needs an update. There's information on that page
which is ~decades out of date (eg Larry Shaffer hasn't been involved in the
project for around 10 years, the OSX packaging team is incorrect, and the
whole "testing" team is non-existent, etc).
I'm happy to help out here with removing old content. My gut feeling is
that a more exhaustive rework is needed and repository / process related
information doesn't belong on this page, and the page should be left to the
"organisation" level governance information alone.
> From experience, I know that the reason why I cannot write to the QGIS
> repository is because "branch protection" is active (for the master branch
> at least) and a set of conditions are required to merge a PR, namely:
>
> - All CI tests need to pass.
>
> - Someone else (I don't know from which group of people) needs to
> approve the proposed change.
Correct. And I would argue that both of these requirements are a valid
**MINIMUM** protection choice for introducing code into a project which has
real world cost impacts for users exceeding millions and potentially
impacting the lives and safety of others.
> 1. CI is often broken for reasons that are independent from the proposed
>change.
Definitely a valid issue, and a real PITA. I'm probably restarting that
mingw workflow ~20x a day for everyone's(!) PRs to keep it limping along.
That said, I'd still rather limp this workflow along vs removing it,
because I do believe that it adds value to our tests and gives end users an
easy way to test PRs prior to merge. I feel the same about the noisy
clang-tidy check: it has a LOT of false positives, but around 1 in 10
failures in that workflow is a valid bug which has been flagged prior to
introducing the code. That's still a net win in my view.
> 2. An aberration of the "review" condition is that a change proposed by a
>contributor and approved by me can be merged but a change proposed by
>me and approved by the same contributor can not be merged, effectively
>giving me ("core QGIS committer") less power than the power of a random
>contributor.
Maybe -- but I would argue that you're looking at the "core contributor"
privilege incorrectly. It's no longer a "you are trusted to put whatever
code you want into the project" vs a "you are trusted to peer review and
approve code proposed for introduction into the project".
Ie **everyone** is on the same level wrt to submitting code for review, but
only the trusted group of core committers are permitted to approve this
code for introduction to QGIS.
> 1. Clearly document the roles and rules on the website
+1
> 2. Allow those with "write access" to self-approve PRs
-1. What's the real motivation here? Why the urgency to get unreviewed code
into QGIS? Again, I am strongly of the opinion that more exhaustive code
merging policies protects our users and their trust in QGIS.
> 3. Define rules by which "write access" privileges to the repository
> are revoked
+1
Nyall
___
QGIS-Developer mailing list
QGIS-Developer@lists.osgeo.org
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
Hi Alessandro, replies inline On Sat, Oct 14, 2023 at 11:17:27AM +0200, Alessandro Pasotti wrote: > I understand your frustration, CI is often failing for no reason and > we are wasting a lot of time (and CPU power) to re-run the failing > workflows, any effort to fix these issues is greatly appreciated. Indeed. It's not always possible to fix them though, as sometimes they just do not depend on us. For example: https://github.com/qgis/QGIS/actions/runs/6509998580/job/17687048243?pr=54923#step:14:12 > Coming to your questions, AFAIK whoever has commit rights can approve > someone else's PR but cannot self approve Do you see how this makes PR coming from non-committers easier to merge ? They basically require 1 committer approval, whereas PR coming from committers require 2 committer approval (the proponent and another reviewer). Beside, is there an official place from which we can read the list of people with "commit rights" ? The term is not in the name or description of any of the github teams: https://github.com/orgs/qgis/teams That term "committer", btw, should probably be changed nowadays as with git it doesnt' make much sense. > The QGIS organization has a budget for PR queue management (triage) > and a separate entry for reviews (budget is public > https://www.qgis.org/en/site/getinvolved/governance/finance/index.html) > , in the past this has been working well while I agree that recently > PRs have been pending without review for a (too) long time. The bugfixing spreadsheet has a column to report reviewer names, but it looks like I'm the only one filling it at the moment. You may notice I've also reported reviews from non-committers ( Laurențiu Nicola, in Cc ). Is there another document (or git commandline) that can be used to track how the review budget is being spent ? > If I am not mistaken, everyone with commit rights can still push to > master directly without opening a PR Unfortunately this is not the case: [strk@c19:/usr/local/src/qgis/qgis/src/master(master)] git push ... remote: error: GH006: Protected branch update failed for refs/heads/master. remote: error: Changes must be made through a pull request. 20 of 20 required status checks are expected. To github.com:qgis/QGIS.git ! [remote rejected] master -> master (protected branch hook declined) error: failed to push some refs to 'github.com:qgis/QGIS.git' > , this is not generally > recommended (I think I have been doing this only once or twice in the > last few years), however a direct push to master is acceptable for a > few cases (e.g. to fix an one-liner change issue where the developer > is fixing his/hers own mess and is 100% sure there will not be side > effects, fix a typo in a comment, for last minute fixes done by the > package maintainers when making the releases etc.). I agree about the usefulness of both reviews and CI in some cases. For GEOS and PostGIS, where we do NOT have enforcement of this rule, we all very often use branches or pull requests or merge requests for the purpose of getting CI and/or peer reviews on our code. It's the enforcement of the rule that's blocking. > A small group of us (3-4 developers including me) have admin access to > all QGIS repositories and we can bypass any check and merge all PRs > without approval. (where) is the composition of this group visible ? Are there documented procedures describing how a "committer" can be added to or removed from this group ? > I admit that I have been using these superpowers more and more often > (IIRC three or four times) in the last year while I have never felt > the need to use them before, in an ideal world it should not be > necessary except for the same use cases I have pointed out for the > direct pushes to master. The world is not ideal. In my ideal world we would not even need to write software, let alone fix bugs ! :P > The bottom line is that the situation is not perfect and can certainly > be improved but at the end of the day we are all doing our best to > keep the process running as smoothly as we can. Be assured I trust the QGIS developers and PSC members to all have good intentions and hope this mail of mine is taken for what it is: a genuine attempt at finding _toghether_ a solution to a problem I see with the currently implemented approval mechanism (unbalanced, in my opinion, toward non-committers). --strk; ___ QGIS-Developer mailing list QGIS-Developer@lists.osgeo.org List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Re: [QGIS-Developer] QGIS repository management
Hi Sandro,
Disclaimer: these are my personal opinions and not the official PSC position.
I understand your frustration, CI is often failing for no reason and
we are wasting a lot of time (and CPU power) to re-run the failing
workflows, any effort to fix these issues is greatly appreciated.
About the merge constraints, I think that they are important and
useful because CI failing tests have proven to be a sign of a real
problem in the past, and also because peer review (PR approval) is
almost always useful to prevent mistakes and ultimately leads to a
better and safer code.
Coming to your questions, AFAIK whoever has commit rights can approve
someone else's PR but cannot self approve (see the exceptions below
about bypassing the blocks).
The QGIS organization has a budget for PR queue management (triage)
and a separate entry for reviews (budget is public
https://www.qgis.org/en/site/getinvolved/governance/finance/index.html)
, in the past this has been working well while I agree that recently
PRs have been pending without review for a (too) long time.
If I am not mistaken, everyone with commit rights can still push to
master directly without opening a PR, this is not generally
recommended (I think I have been doing this only once or twice in the
last few years), however a direct push to master is acceptable for a
few cases (e.g. to fix an one-liner change issue where the developer
is fixing his/hers own mess and is 100% sure there will not be side
effects, fix a typo in a comment, for last minute fixes done by the
package maintainers when making the releases etc.).
A small group of us (3-4 developers including me) have admin access to
all QGIS repositories and we can bypass any check and merge all PRs
without approval.
I admit that I have been using these superpowers more and more often
(IIRC three or four times) in the last year while I have never felt
the need to use them before, in an ideal world it should not be
necessary except for the same use cases I have pointed out for the
direct pushes to master.
The bottom line is that the situation is not perfect and can certainly
be improved but at the end of the day we are all doing our best to
keep the process running as smoothly as we can.
Thank you for raising this point, you are absolutely right that it
should be clearly documented, this has been in our TODO list for a
long time.
Best regards.
On Fri, Oct 13, 2023 at 6:42 PM Sandro Santilli via QGIS-Developer
wrote:
>
> Hello all,
> today I was finally able to more clearly see the problem that frustrates
> me everytime a take part to a new QGIS bugfixing drive, and I would like
> to share it hoping to find a solution togheter.
>
> The main problem:
>
> - Despite having been granted write access to the QGIS repository in 2012
> [1], I cannot effectively use that power today
>
> It's not just me, I think, but I cannot tell for sure because the
> configuration
> of the infrastructure currently in use (github) is not available for me to see
> and the governance page on the official QGIS website does not contain this
> information [2]. This being blind of course adds up to my frustration.
>
> From experience, I know that the reason why I cannot write to the QGIS
> repository is because "branch protection" is active (for the master branch
> at least) and a set of conditions are required to merge a PR, namely:
>
> - All CI tests need to pass.
>
> - Someone else (I don't know from which group of people) needs to
> approve the proposed change.
>
> While I do the above condition being a useful indication for "QGIS
> core developers" to decide whether to accept or not a change request,
> I find them representing an obstacle way more often than a service,
> and in particular:
>
> 1. CI is often broken for reasons that are independent from the proposed
>change.
>
> 2. An aberration of the "review" condition is that a change proposed by a
>contributor and approved by me can be merged but a change proposed by
>me and approved by the same contributor can not be merged, effectively
>giving me ("core QGIS committer") less power than the power of a random
>contributor.
>
> The rules described above are not found from the governance page [2]
> so it isn't easy for me to propose changes because I don't have a clear
> picture of current rules (like, I believe some people in QGIS can
> self-approve PRs but dunno how to tell who and why).
>
> I would personally welcome (and be able to help taking) the following actions:
>
> 1. Clearly document the roles and rules on the website
>
> 2. Allow those with "write access" to self-approve PRs
>
> 3. Define rules by which "write access" privileges to the repository
> are revoked
>
> Thanks for having read this in full, and I hope to hear your position
> on the matter. Happy hacking !
>
>
> [1] https://lists.osgeo.org/pipermail/qgis-developer/2012-October/022715.html
> [2]
[QGIS-Developer] QGIS repository management
Hello all,
today I was finally able to more clearly see the problem that frustrates
me everytime a take part to a new QGIS bugfixing drive, and I would like
to share it hoping to find a solution togheter.
The main problem:
- Despite having been granted write access to the QGIS repository in 2012
[1], I cannot effectively use that power today
It's not just me, I think, but I cannot tell for sure because the configuration
of the infrastructure currently in use (github) is not available for me to see
and the governance page on the official QGIS website does not contain this
information [2]. This being blind of course adds up to my frustration.
>From experience, I know that the reason why I cannot write to the QGIS
repository is because "branch protection" is active (for the master branch
at least) and a set of conditions are required to merge a PR, namely:
- All CI tests need to pass.
- Someone else (I don't know from which group of people) needs to
approve the proposed change.
While I do the above condition being a useful indication for "QGIS
core developers" to decide whether to accept or not a change request,
I find them representing an obstacle way more often than a service,
and in particular:
1. CI is often broken for reasons that are independent from the proposed
change.
2. An aberration of the "review" condition is that a change proposed by a
contributor and approved by me can be merged but a change proposed by
me and approved by the same contributor can not be merged, effectively
giving me ("core QGIS committer") less power than the power of a random
contributor.
The rules described above are not found from the governance page [2]
so it isn't easy for me to propose changes because I don't have a clear
picture of current rules (like, I believe some people in QGIS can
self-approve PRs but dunno how to tell who and why).
I would personally welcome (and be able to help taking) the following actions:
1. Clearly document the roles and rules on the website
2. Allow those with "write access" to self-approve PRs
3. Define rules by which "write access" privileges to the repository
are revoked
Thanks for having read this in full, and I hope to hear your position
on the matter. Happy hacking !
[1] https://lists.osgeo.org/pipermail/qgis-developer/2012-October/022715.html
[2] https://qgis.org/en/site/getinvolved/governance/governance.html
--strk;
Libre GIS consultant/developer
https://strk.kbt.io/services.html
___
QGIS-Developer mailing list
QGIS-Developer@lists.osgeo.org
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
