~sircmpwn/sr.ht-discuss

9 4

builds.sr.ht CI doesn't merge PR's against repo HEAD, unlike other CI services, causing issues

Timothee Cour
Details
Message ID
<CANri+EyJDJmonKyDsiQvTh-M5x_bTiEvnD-dOLHutnq_fefYpg@mail.gmail.com>
DKIM signature
pass
Download raw message
in azure pipelines, github actions, and probably most CI's, PR's are
merged against repo HEAD on every PR push (or upon events such as
closing/reopening). This is an important feature, so that CI reflects
latest state of a repo to prevent bad merge (among other things, it
catches merge conflicts).

in builds.sr.ht CI (which we use for freebsd, openbsd), I just
discovered (after investigating obscure CI failure that was already
fixed in HEAD but was still affecting  pipelines running in
builds.sr.ht exclusively) that this isn't the case: PR's aren't
merged, and CI just checks out the repo at the commit that was pushed
(via printing git rev-parse HEAD).

This results in the best case in occasionally mysterious CI failures
for pipelines running in builds.sr.ht, and in the worst case in making
CI green when they should've failed, causing bad merges.

Can this be fixed in builds.sr.ht (following behavior of other CI's,
ie merge PR's against target branch HEAD on each push and on
closing/reopening) ?

Thanks again for the builds.sr.ht CI which allows us to test against
OS's such as freebsd, openbsd.
Timothee Cour

links:
see https://github.com/nim-lang/Nim/issues/17097
Details
Message ID
<7f6a86c5-ffca-738c-7ae4-9d87b121da70@archlinux.org>
In-Reply-To
<CANri+EyJDJmonKyDsiQvTh-M5x_bTiEvnD-dOLHutnq_fefYpg@mail.gmail.com> (view parent)
DKIM signature
pass
Download raw message
On 2/18/21 9:05 PM, Timothee Cour wrote:
> in azure pipelines, github actions, and probably most CI's, PR's are
> merged against repo HEAD on every PR push (or upon events such as
> closing/reopening). This is an important feature, so that CI reflects
> latest state of a repo to prevent bad merge (among other things, it
> catches merge conflicts).

github already reports merge conflicts in the webui.

I've seen people say *all the time* "please rebase your PR against
master to pick up CI fixes", though I suppose that could just result in
a push event re-triggering CI...

The comparable case is for git.sr.ht, the builds.sr.ht CI applies the
patch to the latest state of the repo -- not unlike merging a PR into
master.

...

Obviously the superior solution is gitlab's concept of PRs that can only
be merged via fast-forward, therefore no need to merge in the CI because
the actual PR commit is the one becoming the new git master commit and
therefore its status is inherently the most relevant.

Given the limitations of github it seems plausible to have CI test the
merge result. :( This could be automatically done via
refs/pull/<number>/merge I guess.

> in builds.sr.ht CI (which we use for freebsd, openbsd), I just
> discovered (after investigating obscure CI failure that was already
> fixed in HEAD but was still affecting  pipelines running in
> builds.sr.ht exclusively) that this isn't the case: PR's aren't
> merged, and CI just checks out the repo at the commit that was pushed
> (via printing git rev-parse HEAD).
> 
> This results in the best case in occasionally mysterious CI failures
> for pipelines running in builds.sr.ht, and in the worst case in making
> CI green when they should've failed, causing bad merges.
> 
> Can this be fixed in builds.sr.ht (following behavior of other CI's,
> ie merge PR's against target branch HEAD on each push and on
> closing/reopening) ?
> 
> Thanks again for the builds.sr.ht CI which allows us to test against
> OS's such as freebsd, openbsd.
> Timothee Cour
> 
> links:
> see https://github.com/nim-lang/Nim/issues/17097
> 


-- 
Eli Schwartz
Bug Wrangler and Trusted User
Details
Message ID
<20210219071443.thmif43dtdqjsypn@hoshi>
In-Reply-To
<CANri+EyJDJmonKyDsiQvTh-M5x_bTiEvnD-dOLHutnq_fefYpg@mail.gmail.com> (view parent)
DKIM signature
missing
Download raw message
On 18-02-2021 18:05:44, Timothee Cour wrote:
> Can this be fixed in builds.sr.ht (following behavior of other CI's,
> ie merge PR's against target branch HEAD on each push and on
> closing/reopening) ?

I wonder: Isn't that possible via the CI script one provides in their
repository?

-- 
Matthias
Details
Message ID
<3452cad4-ec73-4dc0-b20d-da9de717d973@www.fastmail.com>
In-Reply-To
<20210219071443.thmif43dtdqjsypn@hoshi> (view parent)
DKIM signature
pass
Download raw message
It probably is, but the current behaviour is unexpected should one have pre-conceptions based upon other offerings. I set up the CI scripts in question here and didn’t realise this was how builds.sr.ht behaved at all. 

I assume it functions differently for CI for patches (e.g. it behaves the same way as Travis/Azure Pipelines/etc.)?

On Fri, 19 Feb 2021, at 07:14, Matthias Beyer wrote:
> On 18-02-2021 18:05:44, Timothee Cour wrote:
> > Can this be fixed in builds.sr.ht (following behavior of other CI's,
> > ie merge PR's against target branch HEAD on each push and on
> > closing/reopening) ?
> 
> I wonder: Isn't that possible via the CI script one provides in their
> repository?
> 
> -- 
> Matthias
> 
> Attachments:
> * signature.asc
Details
Message ID
<20210219074039.sg5pm53vfsdxu2tp@hoshi>
In-Reply-To
<3452cad4-ec73-4dc0-b20d-da9de717d973@www.fastmail.com> (view parent)
DKIM signature
missing
Download raw message
On 19-02-2021 07:20:53, Euan Torano wrote:
> the current behaviour is unexpected

I disagree. It is not unexpected, it is just not doing anything *implicitely*,
that's a huge difference IMHO.

Right now, from my limited perspective as a mere user of builds, it does _very_
little (install stuff, set ENV, clone the repos and run the script), and I like
that, because it still gives the user power to do more explicitely if they want
to!

I can totally see where you're coming from, though!
Details
Message ID
<05b1dfda-15a7-45cb-9d25-68162a743169@www.fastmail.com>
In-Reply-To
<20210219074039.sg5pm53vfsdxu2tp@hoshi> (view parent)
DKIM signature
pass
Download raw message
I think that if nothing else the documentation could use a call out warning users coming from other services of the difference in behaviour. That may help save some confusion at least. 

I’m going to look at updating the build scripts in question to do the merge into the main branch before running the CI. 

On Fri, 19 Feb 2021, at 07:40, Matthias Beyer wrote:
> On 19-02-2021 07:20:53, Euan Torano wrote:
> > the current behaviour is unexpected
> 
> I disagree. It is not unexpected, it is just not doing anything *implicitely*,
> that's a huge difference IMHO.
> 
> Right now, from my limited perspective as a mere user of builds, it does _very_
> little (install stuff, set ENV, clone the repos and run the script), and I like
> that, because it still gives the user power to do more explicitely if they want
> to!
> 
> I can totally see where you're coming from, though!
> 
> 
> Attachments:
> * signature.asc
Details
Message ID
<C9DK6CCYKEFL.3V1F9S7OU82GS@taiga>
In-Reply-To
<CANri+EyJDJmonKyDsiQvTh-M5x_bTiEvnD-dOLHutnq_fefYpg@mail.gmail.com> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
I think you should be able to achieve this behavior yourself with an
extra task step. Test if you're in a pull request (the environment
variables will help here), and if so, fetch upstream and check out a
branch which looks the way you want. If a merge conflict occurs, fail
the build.

I might be more open to helping you with this if you guys fixed the
OpenBSD utilization issues I've been complaining about for the last few
weeks :)
Details
Message ID
<a2b1d7c7-84e7-4d2b-8af4-cdf2732ff8b5@www.fastmail.com>
In-Reply-To
<C9DK6CCYKEFL.3V1F9S7OU82GS@taiga> (view parent)
DKIM signature
pass
Download raw message
I __think__ I've solved it like this: https://github.com/nim-lang/Nim/pull/17104

I wasn't aware of any complaints RE OpenBSD utilisation issues, but I know that the original build I wrote got split into 3 separate builds because the build was taking a long time on OpenBSD. Any chance you can summarise the issues and I'll report them to Nim and hopefully find some time to look into them?

On Fri, 19 Feb 2021, at 13:57, Drew DeVault wrote:
> I think you should be able to achieve this behavior yourself with an
> extra task step. Test if you're in a pull request (the environment
> variables will help here), and if so, fetch upstream and check out a
> branch which looks the way you want. If a merge conflict occurs, fail
> the build.
> 
> I might be more open to helping you with this if you guys fixed the
> OpenBSD utilization issues I've been complaining about for the last few
> weeks :)
>
Details
Message ID
<C9DLASY3YH7V.3FQI45LWF9O4T@taiga>
In-Reply-To
<a2b1d7c7-84e7-4d2b-8af4-cdf2732ff8b5@www.fastmail.com> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
On Fri Feb 19, 2021 at 9:47 AM EST, Euan Torano wrote:
> I __think__ I've solved it like this:
> https://github.com/nim-lang/Nim/pull/17104

LGTM, but note that your bashisms (`if [[`) may stop working without
notice as we plan on updating each image to use its native shell at some
point in the future (for the BSDs, this would be a more POSIXy shell,
not bash).

> I wasn't aware of any complaints RE OpenBSD utilisation issues, but I
> know that the original build I wrote got split into 3 separate builds
> because the build was taking a long time on OpenBSD. Any chance you can
> summarise the issues and I'll report them to Nim and hopefully find some
> time to look into them?

Long-running builds are less taxing on our resources than running
several builds at once. Nim runs *a lot* of builds, and when every one
includes three redundant OpenBSD jobs, the resource usage is not really
appropriate.

Each VM has 2 CPUs, and you should make sure you utilize both, but
shouldn't have so many build jobs for what is ultimately the same
system. I have to cancel a lot of Nim builds in order to make room for
other users.
Details
Message ID
<8bb53ef5-314f-4dd6-9719-af0c1694ac08@www.fastmail.com>
In-Reply-To
<C9DLASY3YH7V.3FQI45LWF9O4T@taiga> (view parent)
DKIM signature
pass
Download raw message
Good point RE bash-isms, will fix - thanks!

Fair enough. I did originally use a single job but things got changed. I've reported this to the project and will be opening a PR shortly to fix this - the last thing I intended was to cause problems for you or other users! Issue ref: https://github.com/nim-lang/Nim/issues/17107

If there are any other concerns with Nim's usage or anything, please do feel free to contact me. I usually lurk in IRC too if you need a more prompt response.

On Fri, 19 Feb 2021, at 14:50, Drew DeVault wrote:
> On Fri Feb 19, 2021 at 9:47 AM EST, Euan Torano wrote:
> > I __think__ I've solved it like this:
> > https://github.com/nim-lang/Nim/pull/17104
> 
> LGTM, but note that your bashisms (`if [[`) may stop working without
> notice as we plan on updating each image to use its native shell at some
> point in the future (for the BSDs, this would be a more POSIXy shell,
> not bash).
> 
> > I wasn't aware of any complaints RE OpenBSD utilisation issues, but I
> > know that the original build I wrote got split into 3 separate builds
> > because the build was taking a long time on OpenBSD. Any chance you can
> > summarise the issues and I'll report them to Nim and hopefully find some
> > time to look into them?
> 
> Long-running builds are less taxing on our resources than running
> several builds at once. Nim runs *a lot* of builds, and when every one
> includes three redundant OpenBSD jobs, the resource usage is not really
> appropriate.
> 
> Each VM has 2 CPUs, and you should make sure you utilize both, but
> shouldn't have so many build jobs for what is ultimately the same
> system. I have to cancel a lot of Nim builds in order to make room for
> other users.
>
Reply to thread Export thread (mbox)