Direct push forbidden to master

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

Direct push forbidden to master

3nids
Dear all,

We have had the direct push forbidden to master for a bit of time and it proved to be useful.
It apparently had been disabled and a direct push was made which broke master.

I believe it's useless to list the advantages of enforcing the use of pull requests, and I would probably fail to be exhaustive.
But I would like that we take a decision (taken by the dev community) and stick to it.

I understand it is frustrating, but I currently don't see any valid reasons not to respect this:
* releases are made from the release branches (so the recent event on master doesn't seem to apply)
* if we have issues with Windows build, let's investigate if we can have a CI linked to Github for this matter
* if we think the queue is too long, let's talk about subscribing to a paying account on Travis (I am not trolling)

What is the path to take a decision on this i.e. enforcing to go through PR on master?

Cheers,
Denis


_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

ElPaso
Il 08/11/19 09:02, Jürgen E. Fischer ha scritto:

> Hi Denis,
>
> On Fri, 08. Nov 2019 at 08:56:35 +0100, Denis Rouzaud wrote:
>> We have had the direct push forbidden to master for a bit of time and it
>> proved to be useful.
> When did we vote on this?
>
>
>> It apparently had been disabled and a direct push was made which broke
>> master.
> You mean it didn't fully fix it.  Because it was already broken - just travis
> didn't notice.
>
>
>> I believe it's useless to list the advantages of enforcing the use of pull
>> requests, and I would probably fail to be exhaustive.
> -1
>
>
> Jürgen
>

I'm with Jürgen on this, I think we all are responsible developers
acting for the good of QGIS and I didn't see any abuse on direct commits
in the past few years.


On the contrary, I think I should have committed to master directly to
correct a PR of mines that I merged by mistake the day before 3.10
release (https://github.com/qgis/QGIS/pull/32369) instead of waiting for
an approval that didn't arrive in time (in this case it wasn't really a
big problem though).


Cheers


--
Alessandro Pasotti
w3: www.itopen.it

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

3nids


Le ven. 8 nov. 2019 à 09:33, ElPaso <[hidden email]> a écrit :

I'm with Jürgen on this, I think we all are responsible developers
acting for the good of QGIS and I didn't see any abuse on direct commits
in the past few years.

While not strictly talking about abuse, there has been quite a few direct push commits which broke the CI.
That generally meant looking at someone else work to fix things, sometimes by several persons at the same time.
It also means all last PRs jobs that have to be restarted.
It gives a bad image/impression.
It's frustrating.

One saves himself about one hour time to see his work integrated in the final branch.
All the good soldiers in the Travis queue are good for a restart.

In the history of breaking commits, it's always "small and insignificant changes which won't affect the build or the CI". I made a few of these in the past too.
Just because we don't have docker and clang installed in our brains ;)

What are the valid arguments to allow direct pushes?
I can see only one: the release process might requires this since it relies on the branches themselves.
But this was clearly not the case in the last event.


On the contrary, I think I should have committed to master directly to
correct a PR of mines that I merged by mistake the day before 3.10
release (https://github.com/qgis/QGIS/pull/32369) instead of waiting for
an approval that didn't arrive in time (in this case it wasn't really a
big problem though).

OK, this might be a valid reason: to fix merge errors.
But this really sounds like an edge case to me. 
Anyway, this directly fit in the release process window I was mentioning above, which might require a special treatment.

So this could be a proposition.
Enforce PRs in standard time but allow direct push by a list of safe core-devs during the release process window.

Cheers,
Denis

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Nyall Dawson
In reply to this post by ElPaso
On Fri, 8 Nov 2019 at 18:33, ElPaso <[hidden email]> wrote:

> I'm with Jürgen on this, I think we all are responsible developers
> acting for the good of QGIS and I didn't see any abuse on direct commits
> in the past few years.
>
>
> On the contrary, I think I should have committed to master directly to
> correct a PR of mines that I merged by mistake the day before 3.10
> release (https://github.com/qgis/QGIS/pull/32369) instead of waiting for
> an approval that didn't arrive in time (in this case it wasn't really a
> big problem though).

I don't think Denis is arguing for forced-reviews of pull requests.
Rather just that all changes GO through pull requests so that we can
be sure they don't break CI.

Nyall

>
>
> Cheers
>
>
> --
> Alessandro Pasotti
> w3: www.itopen.it
>
> _______________________________________________
> Qgis-psc mailing list
> [hidden email]
> https://lists.osgeo.org/mailman/listinfo/qgis-psc
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Nyall Dawson
In reply to this post by 3nids
On Fri, 8 Nov 2019 at 19:04, Denis Rouzaud <[hidden email]> wrote:

>>
>> I'm with Jürgen on this, I think we all are responsible developers
>> acting for the good of QGIS and I didn't see any abuse on direct commits
>> in the past few years.
>
>
> While not strictly talking about abuse, there has been quite a few direct push commits which broke the CI.
> That generally meant looking at someone else work to fix things, sometimes by several persons at the same time.
> It also means all last PRs jobs that have to be restarted.
> It gives a bad image/impression.
> It's frustrating.
>
> One saves himself about one hour time to see his work integrated in the final branch.
> All the good soldiers in the Travis queue are good for a restart

I'm +1 to forced use of pull requests and no direct pushes to master.
The reality is that breakage of CI today cost me quite a lot of wasted
time, when I had a very narrow window to make some bug fixes and see
that they would pass the CI tests before leaving for FOSS4G Oceania
next week.

My view is that it's just not a fair practice for other community
members -- while it avoids the submitter having to wait for the CI
checks to pass, the cost is the risk of causing large disruption to
many other developer's work.

If we all play nice and follow this practice then everyone is
following the same rules. If we relax it and allow force pushes from
some developers then we have inequality. And if we relax it for
everyone, we go back to the anarchy which development was in the early
2.x days (just thinking back on this gives me nightmares. I honestly
can't believe now looking back that we all (myself included) used to
push 1k+ line feature changes direct to master with zero code review
or CI tests. Eeeeeeeeeeeeeeeeee)

Nyall

>
> In the history of breaking commits, it's always "small and insignificant changes which won't affect the build or the CI". I made a few of these in the past too.
> Just because we don't have docker and clang installed in our brains ;)
>
> What are the valid arguments to allow direct pushes?
> I can see only one: the release process might requires this since it relies on the branches themselves.
> But this was clearly not the case in the last event.
>
>>
>> On the contrary, I think I should have committed to master directly to
>> correct a PR of mines that I merged by mistake the day before 3.10
>> release (https://github.com/qgis/QGIS/pull/32369) instead of waiting for
>> an approval that didn't arrive in time (in this case it wasn't really a
>> big problem though).
>
>
> OK, this might be a valid reason: to fix merge errors.
> But this really sounds like an edge case to me.
> Anyway, this directly fit in the release process window I was mentioning above, which might require a special treatment.
>
> So this could be a proposition.
> Enforce PRs in standard time but allow direct push by a list of safe core-devs during the release process window.
>
> Cheers,
> Denis
> _______________________________________________
> Qgis-psc mailing list
> [hidden email]
> https://lists.osgeo.org/mailman/listinfo/qgis-psc
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Even Rouault-2
In reply to this post by Nyall Dawson
On vendredi 8 novembre 2019 19:32:06 CET Nyall Dawson wrote:

> On Fri, 8 Nov 2019 at 18:33, ElPaso <[hidden email]> wrote:
> > I'm with Jürgen on this, I think we all are responsible developers
> > acting for the good of QGIS and I didn't see any abuse on direct commits
> > in the past few years.
> >
> >
> > On the contrary, I think I should have committed to master directly to
> > correct a PR of mines that I merged by mistake the day before 3.10
> > release (https://github.com/qgis/QGIS/pull/32369) instead of waiting for
> > an approval that didn't arrive in time (in this case it wasn't really a
> > big problem though).
>
> I don't think Denis is arguing for forced-reviews of pull requests.
> Rather just that all changes GO through pull requests so that we can
> be sure they don't break CI.

What is obvious here is that a CI for Windows would avoid avoided this case,
as the cost of maintaining the Windows build alive mostly relies on a single
person currently.
As it seems that most QGIS users run Windows, it might be worth for the
project to invest into that, both in funding the time of the folks who would
set up that and possibly paying for a beefy enough server (potentially from a
commercial CI hosting solution) that would sustain the load of pull requests.

And paying for Travis extra build could potentially save time for a number of
contributors. For the github OSGeo organization, mostly used by PROJ & GDAL,
it is between 4000-5000 USD/year for 11 parallel builds (this includes a
discount for OSGeo being a non-profit). There might be cheeper alternatives by
other competitors.

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Nathan Woodrow
I agree with Even.  The issue here is the slow CI builds and lack of Windows CI builds.  
To make this process smoother and more solid for everyone we need to invest in better CI not pretend we can just ignore it for the sake of getting a commit in..

- Nathan

On Fri, Nov 8, 2019 at 10:05 PM Even Rouault <[hidden email]> wrote:
On vendredi 8 novembre 2019 19:32:06 CET Nyall Dawson wrote:
> On Fri, 8 Nov 2019 at 18:33, ElPaso <[hidden email]> wrote:
> > I'm with Jürgen on this, I think we all are responsible developers
> > acting for the good of QGIS and I didn't see any abuse on direct commits
> > in the past few years.
> >
> >
> > On the contrary, I think I should have committed to master directly to
> > correct a PR of mines that I merged by mistake the day before 3.10
> > release (https://github.com/qgis/QGIS/pull/32369) instead of waiting for
> > an approval that didn't arrive in time (in this case it wasn't really a
> > big problem though).
>
> I don't think Denis is arguing for forced-reviews of pull requests.
> Rather just that all changes GO through pull requests so that we can
> be sure they don't break CI.

What is obvious here is that a CI for Windows would avoid avoided this case,
as the cost of maintaining the Windows build alive mostly relies on a single
person currently.
As it seems that most QGIS users run Windows, it might be worth for the
project to invest into that, both in funding the time of the folks who would
set up that and possibly paying for a beefy enough server (potentially from a
commercial CI hosting solution) that would sustain the load of pull requests.

And paying for Travis extra build could potentially save time for a number of
contributors. For the github OSGeo organization, mostly used by PROJ & GDAL,
it is between 4000-5000 USD/year for 11 parallel builds (this includes a
discount for OSGeo being a non-profit). There might be cheeper alternatives by
other competitors.

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Matthias Kuhn 🌍

Hi,

I think this discussion was triggered partially at least by my refactoring of the cmake build system. I am sorry for any inconveniences that this triggered and hope it's justified by the long term simplification that should come out of this.

Overall I see three different topics. One is the request for a Windows based CI, one is a rant over travis and CI in general and one is the question if direct pushes to master should be allowed (and encouraged).

A Windows based CI would in my opinion be highly desirable. I cannot see any reason not to do it actually. Most users run QGIS on Windows, that's just the reality. And we want QGIS to run stable on Windows. The questions that remain is how much $$ the infrastructure and it's setup is and if we get a Windows build system to complete in a reasonable time. I don't think nightly builds are equivalent, last week they were broken and it was up to one single person (Jürgen) to fix this. If a CI had been in place it would have been my job to fix the build before it is merged into our codebase, which would have been much more convenient for everyone. We also don't need all tests to pass on Windows to put this into play. We can blacklist some tests and gradually fix remaining ones. We have done that before on other platforms too.

If Travis (or any CI) is worth the effort is indeed a question that can and should be asked. It does require a lot of time to keep it up and running. What it provides on the other hand is an early feedback of regressions. And early in this case means detecting them before they even are in the codebase. This saves valuable time from our testers which can instead focus on testing things which have not been detected by the CI. And it saves time of core developers who don't need to fix bugs introduced by others. It also makes it easier to fix bugs because they are related to a particular pull request and context. Yes, travis also bitches around sometime and makes us loose time. I can't give a definite answer but in comparison with the wild west style before I am in favor of our CI.

Pushing directly to master is not a complete disaster, but I can't see many advantages of doing it. So far it's sort of a gentlemen agreement that most don't push directly to master. I don't remember the last time I did it actually. I am fine with restricting and enforcing this on github but if that's not desired by the PSC / Jürgen, I would like to appeal to everyone to keep away from doing direct pushes. Waiting an hour for a green tick shouldn't be asked too much.

-- Matthias

On 11/8/19 1:23 PM, Nathan Woodrow wrote:
I agree with Even.  The issue here is the slow CI builds and lack of Windows CI builds.  
To make this process smoother and more solid for everyone we need to invest in better CI not pretend we can just ignore it for the sake of getting a commit in..

- Nathan

On Fri, Nov 8, 2019 at 10:05 PM Even Rouault <[hidden email]> wrote:
On vendredi 8 novembre 2019 19:32:06 CET Nyall Dawson wrote:
> On Fri, 8 Nov 2019 at 18:33, ElPaso <[hidden email]> wrote:
> > I'm with Jürgen on this, I think we all are responsible developers
> > acting for the good of QGIS and I didn't see any abuse on direct commits
> > in the past few years.
> >
> >
> > On the contrary, I think I should have committed to master directly to
> > correct a PR of mines that I merged by mistake the day before 3.10
> > release (https://github.com/qgis/QGIS/pull/32369) instead of waiting for
> > an approval that didn't arrive in time (in this case it wasn't really a
> > big problem though).
>
> I don't think Denis is arguing for forced-reviews of pull requests.
> Rather just that all changes GO through pull requests so that we can
> be sure they don't break CI.

What is obvious here is that a CI for Windows would avoid avoided this case,
as the cost of maintaining the Windows build alive mostly relies on a single
person currently.
As it seems that most QGIS users run Windows, it might be worth for the
project to invest into that, both in funding the time of the folks who would
set up that and possibly paying for a beefy enough server (potentially from a
commercial CI hosting solution) that would sustain the load of pull requests.

And paying for Travis extra build could potentially save time for a number of
contributors. For the github OSGeo organization, mostly used by PROJ & GDAL,
it is between 4000-5000 USD/year for 11 parallel builds (this includes a
discount for OSGeo being a non-profit). There might be cheeper alternatives by
other competitors.

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Alessandro Pasotti-2


On Sat, Nov 9, 2019 at 2:48 PM Matthias Kuhn <[hidden email]> wrote:

Hi,

I think this discussion was triggered partially at least by my refactoring of the cmake build system. I am sorry for any inconveniences that this triggered and hope it's justified by the long term simplification that should come out of this.

Overall I see three different topics. One is the request for a Windows based CI, one is a rant over travis and CI in general and one is the question if direct pushes to master should be allowed (and encouraged).

A Windows based CI would in my opinion be highly desirable. I cannot see any reason not to do it actually. Most users run QGIS on Windows, that's just the reality. And we want QGIS to run stable on Windows. The questions that remain is how much $$ the infrastructure and it's setup is and if we get a Windows build system to complete in a reasonable time. I don't think nightly builds are equivalent, last week they were broken and it was up to one single person (Jürgen) to fix this. If a CI had been in place it would have been my job to fix the build before it is merged into our codebase, which would have been much more convenient for everyone. We also don't need all tests to pass on Windows to put this into play. We can blacklist some tests and gradually fix remaining ones. We have done that before on other platforms too.

If Travis (or any CI) is worth the effort is indeed a question that can and should be asked. It does require a lot of time to keep it up and running. What it provides on the other hand is an early feedback of regressions. And early in this case means detecting them before they even are in the codebase. This saves valuable time from our testers which can instead focus on testing things which have not been detected by the CI. And it saves time of core developers who don't need to fix bugs introduced by others. It also makes it easier to fix bugs because they are related to a particular pull request and context. Yes, travis also bitches around sometime and makes us loose time. I can't give a definite answer but in comparison with the wild west style before I am in favor of our CI.

Pushing directly to master is not a complete disaster, but I can't see many advantages of doing it. So far it's sort of a gentlemen agreement that most don't push directly to master. I don't remember the last time I did it actually. I am fine with restricting and enforcing this on github but if that's not desired by the PSC / Jürgen, I would like to appeal to everyone to keep away from doing direct pushes. Waiting an hour for a green tick shouldn't be asked too much.



Hi Matthias,

I totally agree with your last claim: I think we must all go through PRs and this is what "normally" everyone does, but I see a very few and limited cases when a direct commit to master is the right thing to do and I recommend to leave that door open.

A stupid example is fixing a typo in a comment, to be honest if I need to branch and create a PR and go through Travis I will just leave the typo (if that's not part of something I'm still working on). Of course, sticking to this stupid example, this also means that after fixing this typo in the comment if master is broken I won't go to sleep until I fixed it.

As I've already mentioned I don't see a problem here, 99.99% of the workflow goes through Travis and in the few exceptional cases when it doesn't, if master was broken it was fixed almost immediately, in any event I don't see a big issue if master is broken for some time provided that who breaks it also takes the responsibility to fix it quickly.

Speaking of Travis, I've found it a life-saver many times but I also think that given the amount of time we spent on it we could have rewritten QGIS in nodejs (because this is the long term plan, isn't it?)  but without a better alternative we must live with it.

Just one thing I would recommend investing a bit more time and money: to be able to run a Travis-like test suite locally (I was able to do it but it costed me a lot of time to set up and to understand how to use it, I definitely don't use it regularly), here are the reasons:
- branch switch costs time, and I'm often working on several fixes/features at the same time, waiting for Travis to know that it failed because of (say) a silly typo in a comment is a huge waste of time
- sometimes being able to debug the Travis locally is the only option (I know there is  way to ssh into remote Travis but that's a bit complicated and slow)
- my machine is faster than Travis
- prepare-commit doesn't make all checks (spelling, others?)

Cutting down build times would also help but I don't have any real proposal here, I've read somewhere that other build systems might be faster and that pre-compiled header might help but I'm not an expert in this field

So, IMHO long life to PRs and Travis for the time being but we need to give to the developers the tools to run locally all checks that Travis runs remotely so that they are reasonably sure ("reasonably" because Travis random unrelated failures are always around the corner) that the build will pass and they will be able to debug a Travis failure locally without waiting for hours.

Cheers


-- Matthias

On 11/8/19 1:23 PM, Nathan Woodrow wrote:
I agree with Even.  The issue here is the slow CI builds and lack of Windows CI builds.  
To make this process smoother and more solid for everyone we need to invest in better CI not pretend we can just ignore it for the sake of getting a commit in..

- Nathan

On Fri, Nov 8, 2019 at 10:05 PM Even Rouault <[hidden email]> wrote:
On vendredi 8 novembre 2019 19:32:06 CET Nyall Dawson wrote:
> On Fri, 8 Nov 2019 at 18:33, ElPaso <[hidden email]> wrote:
> > I'm with Jürgen on this, I think we all are responsible developers
> > acting for the good of QGIS and I didn't see any abuse on direct commits
> > in the past few years.
> >
> >
> > On the contrary, I think I should have committed to master directly to
> > correct a PR of mines that I merged by mistake the day before 3.10
> > release (https://github.com/qgis/QGIS/pull/32369) instead of waiting for
> > an approval that didn't arrive in time (in this case it wasn't really a
> > big problem though).
>
> I don't think Denis is arguing for forced-reviews of pull requests.
> Rather just that all changes GO through pull requests so that we can
> be sure they don't break CI.

What is obvious here is that a CI for Windows would avoid avoided this case,
as the cost of maintaining the Windows build alive mostly relies on a single
person currently.
As it seems that most QGIS users run Windows, it might be worth for the
project to invest into that, both in funding the time of the folks who would
set up that and possibly paying for a beefy enough server (potentially from a
commercial CI hosting solution) that would sustain the load of pull requests.

And paying for Travis extra build could potentially save time for a number of
contributors. For the github OSGeo organization, mostly used by PROJ & GDAL,
it is between 4000-5000 USD/year for 11 parallel builds (this includes a
discount for OSGeo being a non-profit). There might be cheeper alternatives by
other competitors.

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc


--
Alessandro Pasotti
w3:   www.itopen.it

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Sandro Santilli-4
On Sun, Nov 10, 2019 at 09:20:11AM +0100, Alessandro Pasotti wrote:

> Just one thing I would recommend investing a bit more time and money: to be
> able to run a Travis-like test suite locally

+1

I can't remember the last time I had a successful `make check`, bust
must be decades ago... Improving the ability for devs to run testsuite
locally is a huge asset. I think it goes along the lines of:

  - Only run tests that can be run on the machine, given the available deps
    ... and WARN at build/test time about tests being skipped,
    to hint developer about improving the test coverage

  - Ensure running the tests doesn't mess up with your worktree
    See https://github.com/qgis/QGIS/pull/31980

  - Make it easy for devs to run single, specific tests, to help
    when changing a single specific spot of the codebase

> So, IMHO long life to PRs and Travis for the time being but we need to give
> to the developers the tools to run locally all checks that Travis runs
> remotely so that they are reasonably sure ("reasonably" because Travis
> random unrelated failures are always around the corner) that the build will
> pass and they will be able to debug a Travis failure locally without
> waiting for hours.

Now I realize you're talking about travis-like, that's also something
good to have, but as it would likely take much more upfront bandwidth
(docker image download?) and space (docker image) and time, it is also
good to support more lightweight testing.

--strk;
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Tim Sutton-6
In reply to this post by Alessandro Pasotti-2
Hi


Just one thing I would recommend investing a bit more time and money: to be able to run a Travis-like test suite locally (I was able to do it but it costed me a lot of time to set up and to understand how to use it, I definitely don't use it regularly), here are the reasons:
- branch switch costs time, and I'm often working on several fixes/features at the same time, waiting for Travis to know that it failed because of (say) a silly typo in a comment is a huge waste of time
- sometimes being able to debug the Travis locally is the only option (I know there is  way to ssh into remote Travis but that's a bit complicated and slow)
- my machine is faster than Travis
- prepare-commit doesn't make all checks (spelling, others?)

Cutting down build times would also help but I don't have any real proposal here, I've read somewhere that other build systems might be faster and that pre-compiled header might help but I'm not an expert in this field

So, IMHO long life to PRs and Travis for the time being but we need to give to the developers the tools to run locally all checks that Travis runs remotely so that they are reasonably sure ("reasonably" because Travis random unrelated failures are always around the corner) that the build will pass and they will be able to debug a Travis failure locally without waiting for hours.


I know we have raised the idea before and then blown it away, but it may be worth resurfacing: What about splitting QGIS up into a number of discrete libraries + the desktop app? Each could be in it’s own repo with it’s own CI and test suite. I know there are downsides to the approach (e.g. where should I report a bug?). We could still ‘fake’ the current QGIS repo using submodules so that the packaging etc. workflows are minimally affected (and maybe can take advantage of quick tweaks to packaging scripts etc without needing to wait for CI checks). Also developers wanting to work on e.g. providers could work in a more isolated environment. Anyway I know there were also good reasons not to do this so just raising the thought again in case over time it becomes more interesting.

Regards

Tim



Cheers






‘''






Tim Sutton

Co-founder: Kartoza
Ex Project chair: QGIS.org

Visit http://kartoza.com to find out about open source:

Desktop GIS programming services
Geospatial web development
GIS Training
Consulting Services

Skype: timlinux 
IRC: timlinux on #qgis at freenode.net

I'd love to connect. Here's my calendar link to make finding time easy.


_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Matthias Kuhn 🌍
In reply to this post by Sandro Santilli-4
On 11/10/19 11:42 AM, Sandro Santilli wrote:
On Sun, Nov 10, 2019 at 09:20:11AM +0100, Alessandro Pasotti wrote:

Just one thing I would recommend investing a bit more time and money: to be
able to run a Travis-like test suite locally
+1

I can't remember the last time I had a successful `make check`, bust
must be decades ago... Improving the ability for devs to run testsuite
locally is a huge asset. I think it goes along the lines of:

  - Only run tests that can be run on the machine, given the available deps
    ... and WARN at build/test time about tests being skipped,
    to hint developer about improving the test coverage

  - Ensure running the tests doesn't mess up with your worktree
    See https://github.com/qgis/QGIS/pull/31980

  - Make it easy for devs to run single, specific tests, to help
    when changing a single specific spot of the codebase

See https://docs.qgis.org/testing/en/docs/developers_guide/unittesting.html#run-your-tests , it says

> If a test fails, you can use the ctest command to examine more closely why it failed. Use the -R option to specify a regex for which tests you want to run and -V to get verbose output:

> $ ctest -R appl -V

Is there more required?


So, IMHO long life to PRs and Travis for the time being but we need to give
to the developers the tools to run locally all checks that Travis runs
remotely so that they are reasonably sure ("reasonably" because Travis
random unrelated failures are always around the corner) that the build will
pass and they will be able to debug a Travis failure locally without
waiting for hours.
Now I realize you're talking about travis-like, that's also something
good to have, but as it would likely take much more upfront bandwidth
(docker image download?) and space (docker image) and time, it is also
good to support more lightweight testing.

--strk;

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Matthias Kuhn 🌍
In reply to this post by Alessandro Pasotti-2

Hi Alessandro

On 11/10/19 9:20 AM, Alessandro Pasotti wrote:

On Sat, Nov 9, 2019 at 2:48 PM Matthias Kuhn <[hidden email]> wrote:

Hi,

I think this discussion was triggered partially at least by my refactoring of the cmake build system. I am sorry for any inconveniences that this triggered and hope it's justified by the long term simplification that should come out of this.

Overall I see three different topics. One is the request for a Windows based CI, one is a rant over travis and CI in general and one is the question if direct pushes to master should be allowed (and encouraged).

A Windows based CI would in my opinion be highly desirable. I cannot see any reason not to do it actually. Most users run QGIS on Windows, that's just the reality. And we want QGIS to run stable on Windows. The questions that remain is how much $$ the infrastructure and it's setup is and if we get a Windows build system to complete in a reasonable time. I don't think nightly builds are equivalent, last week they were broken and it was up to one single person (Jürgen) to fix this. If a CI had been in place it would have been my job to fix the build before it is merged into our codebase, which would have been much more convenient for everyone. We also don't need all tests to pass on Windows to put this into play. We can blacklist some tests and gradually fix remaining ones. We have done that before on other platforms too.

If Travis (or any CI) is worth the effort is indeed a question that can and should be asked. It does require a lot of time to keep it up and running. What it provides on the other hand is an early feedback of regressions. And early in this case means detecting them before they even are in the codebase. This saves valuable time from our testers which can instead focus on testing things which have not been detected by the CI. And it saves time of core developers who don't need to fix bugs introduced by others. It also makes it easier to fix bugs because they are related to a particular pull request and context. Yes, travis also bitches around sometime and makes us loose time. I can't give a definite answer but in comparison with the wild west style before I am in favor of our CI.

Pushing directly to master is not a complete disaster, but I can't see many advantages of doing it. So far it's sort of a gentlemen agreement that most don't push directly to master. I don't remember the last time I did it actually. I am fine with restricting and enforcing this on github but if that's not desired by the PSC / Jürgen, I would like to appeal to everyone to keep away from doing direct pushes. Waiting an hour for a green tick shouldn't be asked too much.



Hi Matthias,

I totally agree with your last claim: I think we must all go through PRs and this is what "normally" everyone does, but I see a very few and limited cases when a direct commit to master is the right thing to do and I recommend to leave that door open.

A stupid example is fixing a typo in a comment, to be honest if I need to branch and create a PR and go through Travis I will just leave the typo (if that's not part of something I'm still working on). Of course, sticking to this stupid example, this also means that after fixing this typo in the comment if master is broken I won't go to sleep until I fixed it.

I am sorry to disagree on this first part. I don't think this warrants a direct push. I don't even think it saves time. I usually don't have a clean master checked out in this scenario, so I still need to add a separate worktree where I can cherry-pick something and then I can as well just push it to a new PR branch that lets me figure out if this introduced a new typo or if I forgot to sync the changes to sip.


As I've already mentioned I don't see a problem here, 99.99% of the workflow goes through Travis and in the few exceptional cases when it doesn't, if master was broken it was fixed almost immediately, in any event I don't see a big issue if master is broken for some time provided that who breaks it also takes the responsibility to fix it quickly.

Speaking of Travis, I've found it a life-saver many times but I also think that given the amount of time we spent on it we could have rewritten QGIS in nodejs (because this is the long term plan, isn't it?)  but without a better alternative we must live with it.

Just one thing I would recommend investing a bit more time and money: to be able to run a Travis-like test suite locally (I was able to do it but it costed me a lot of time to set up and to understand how to use it, I definitely don't use it regularly), here are the reasons:
- branch switch costs time, and I'm often working on several fixes/features at the same time, waiting for Travis to know that it failed because of (say) a silly typo in a comment is a huge waste of time
- sometimes being able to debug the Travis locally is the only option (I know there is  way to ssh into remote Travis but that's a bit complicated and slow)
- my machine is faster than Travis
- prepare-commit doesn't make all checks (spelling, others?)

Integrating more static tests in the pre-commit hook sounds like a very good idea (i.e. the typo checks you mention).

It would also be nice to improve the possibilities to run the CI containers locally. It will still be tough to debug because it's not directly on the system but inside a container (i.e. no direct IDE debugger integration), but I can definitely see some advantages there.


Cutting down build times would also help but I don't have any real proposal here, I've read somewhere that other build systems might be faster and that pre-compiled header might help but I'm not an expert in this field

There's some interesting recent information on this too https://www.qt.io/blog/2019/08/01/precompiled-headers-and-unity-jumbo-builds-in-upcoming-cmake


So, IMHO long life to PRs and Travis for the time being but we need to give to the developers the tools to run locally all checks that Travis runs remotely so that they are reasonably sure ("reasonably" because Travis random unrelated failures are always around the corner) that the build will pass and they will be able to debug a Travis failure locally without waiting for hours.

Cheers


-- Matthias

On 11/8/19 1:23 PM, Nathan Woodrow wrote:
I agree with Even.  The issue here is the slow CI builds and lack of Windows CI builds.  
To make this process smoother and more solid for everyone we need to invest in better CI not pretend we can just ignore it for the sake of getting a commit in..

- Nathan

On Fri, Nov 8, 2019 at 10:05 PM Even Rouault <[hidden email]> wrote:
On vendredi 8 novembre 2019 19:32:06 CET Nyall Dawson wrote:
> On Fri, 8 Nov 2019 at 18:33, ElPaso <[hidden email]> wrote:
> > I'm with Jürgen on this, I think we all are responsible developers
> > acting for the good of QGIS and I didn't see any abuse on direct commits
> > in the past few years.
> >
> >
> > On the contrary, I think I should have committed to master directly to
> > correct a PR of mines that I merged by mistake the day before 3.10
> > release (https://github.com/qgis/QGIS/pull/32369) instead of waiting for
> > an approval that didn't arrive in time (in this case it wasn't really a
> > big problem though).
>
> I don't think Denis is arguing for forced-reviews of pull requests.
> Rather just that all changes GO through pull requests so that we can
> be sure they don't break CI.

What is obvious here is that a CI for Windows would avoid avoided this case,
as the cost of maintaining the Windows build alive mostly relies on a single
person currently.
As it seems that most QGIS users run Windows, it might be worth for the
project to invest into that, both in funding the time of the folks who would
set up that and possibly paying for a beefy enough server (potentially from a
commercial CI hosting solution) that would sustain the load of pull requests.

And paying for Travis extra build could potentially save time for a number of
contributors. For the github OSGeo organization, mostly used by PROJ & GDAL,
it is between 4000-5000 USD/year for 11 parallel builds (this includes a
discount for OSGeo being a non-profit). There might be cheeper alternatives by
other competitors.

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc


--
Alessandro Pasotti
w3:   www.itopen.it

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Alessandro Pasotti-2
In reply to this post by Matthias Kuhn 🌍


On Sun, Nov 10, 2019 at 11:54 AM Matthias Kuhn <[hidden email]> wrote:
On 11/10/19 11:42 AM, Sandro Santilli wrote:
On Sun, Nov 10, 2019 at 09:20:11AM +0100, Alessandro Pasotti wrote:

Just one thing I would recommend investing a bit more time and money: to be
able to run a Travis-like test suite locally
+1

I can't remember the last time I had a successful `make check`, bust
must be decades ago... Improving the ability for devs to run testsuite
locally is a huge asset. I think it goes along the lines of:

  - Only run tests that can be run on the machine, given the available deps
    ... and WARN at build/test time about tests being skipped,
    to hint developer about improving the test coverage

  - Ensure running the tests doesn't mess up with your worktree
    See https://github.com/qgis/QGIS/pull/31980

  - Make it easy for devs to run single, specific tests, to help
    when changing a single specific spot of the codebase

See https://docs.qgis.org/testing/en/docs/developers_guide/unittesting.html#run-your-tests , it says

> If a test fails, you can use the ctest command to examine more closely why it failed. Use the -R option to specify a regex for which tests you want to run and -V to get verbose output:

> $ ctest -R appl -V

Is there more required?


That's unfortunately not enough, what I was talking about it run test locally in the exactly same environment that Travis uses, I tried to document part of the workflow in https://github.com/qgis/QGIS/tree/master/.docker but that's not enough to run the tests (I mean all of them including spelling, clazy and all the other checks that are run on Travis and that are not in the pre-commit hook).

There are times when being able to run Travis locally and interactively ssh into the containers a make changes is the only option.

So, first thing is to have a pre-commit that runs **all** the checks that are run on Travis, second is to be able to execute Travis environment locally.

 

      
So, IMHO long life to PRs and Travis for the time being but we need to give
to the developers the tools to run locally all checks that Travis runs
remotely so that they are reasonably sure ("reasonably" because Travis
random unrelated failures are always around the corner) that the build will
pass and they will be able to debug a Travis failure locally without
waiting for hours.
Now I realize you're talking about travis-like, that's also something
good to have, but as it would likely take much more upfront bandwidth
(docker image download?) and space (docker image) and time, it is also
good to support more lightweight testing.

--strk;


--
Alessandro Pasotti
w3:   www.itopen.it

_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Matthias Kuhn 🌍
On 11/10/19 12:49 PM, Alessandro Pasotti wrote:


On Sun, Nov 10, 2019 at 11:54 AM Matthias Kuhn <[hidden email]> wrote:
On 11/10/19 11:42 AM, Sandro Santilli wrote:
On Sun, Nov 10, 2019 at 09:20:11AM +0100, Alessandro Pasotti wrote:

Just one thing I would recommend investing a bit more time and money: to be
able to run a Travis-like test suite locally
+1

I can't remember the last time I had a successful `make check`, bust
must be decades ago... Improving the ability for devs to run testsuite
locally is a huge asset. I think it goes along the lines of:

  - Only run tests that can be run on the machine, given the available deps
    ... and WARN at build/test time about tests being skipped,
    to hint developer about improving the test coverage

  - Ensure running the tests doesn't mess up with your worktree
    See https://github.com/qgis/QGIS/pull/31980

  - Make it easy for devs to run single, specific tests, to help
    when changing a single specific spot of the codebase

See https://docs.qgis.org/testing/en/docs/developers_guide/unittesting.html#run-your-tests , it says

> If a test fails, you can use the ctest command to examine more closely why it failed. Use the -R option to specify a regex for which tests you want to run and -V to get verbose output:

> $ ctest -R appl -V

Is there more required?


That's unfortunately not enough, what I was talking about it run test locally in the exactly same environment that Travis uses, I tried to document part of the workflow in https://github.com/qgis/QGIS/tree/master/.docker but that's not enough to run the tests (I mean all of them including spelling, clazy and all the other checks that are run on Travis and that are not in the pre-commit hook).

There are times when being able to run Travis locally and interactively ssh into the containers a make changes is the only option.

So, first thing is to have a pre-commit that runs **all** the checks that are run on Travis, second is to be able to execute Travis environment locally.


Sorry, I was a bit unclear. That was just meant for the last item in your list.



_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc
Reply | Threaded
Open this post in threaded view
|

Re: [QGIS-Developer] Direct push forbidden to master

Sandro Santilli-4
On Sun, Nov 10, 2019 at 12:51:34PM +0100, Matthias Kuhn wrote:

> Sorry, I was a bit unclear. That was just meant for the last item in your
> list.

For clarity: the list was mine :)

I do undertand Alessandro's point: being able to run
the _same_ tests run by the CI guarding protected branches.

This would be easy with Drone-CI (free software).
There's a public cloud.drone.io service which does work
against github, if we want to try going that way.

Advantage would be that running the _same_ tests would
be as simple as running:

  drone exec

From within the source tree.

Installing "drone" is a matter of instaling a single
binary, or, if you have `go` dev environment setup,
running a single `go install` command.

Reference: https://drone.io/

--strk;
_______________________________________________
Qgis-psc mailing list
[hidden email]
https://lists.osgeo.org/mailman/listinfo/qgis-psc