search

cake-build / cake

:cake: Cake (C# Make) is a cross platform build automation system.
https://cakebuild.net
MIT License
3.89k stars 726 forks source link

Change appveyor.exe to be run as a tool #1112

Open gep13 opened 8 years ago

gep13 commented 8 years ago

Right now, Cake simply shells out to the the underlying appveyor.exe. As a result, silent failures in things like UploadTestResults are not caught.

We should correctly handle appveyor.exe as a Tool, so that exit codes are correctly handled.

This was discussed here: https://github.com/cake-build/cake/issues/1097#issuecomment-234791625

devlead commented 7 years ago

@agc93 just checking, will you be able to look at this in a near future or should we bump it to next release?

agc93 commented 7 years ago

Probably bump, sorry 😢 I'm looking at a couple of the VS issues at the moment.

devlead commented 7 years ago

@agc93 no worries, bumped it to v0.19.0

agc93 commented 7 years ago

Hoping for an opinion from @patriksvensson / @devlead / @gep13 here :smile:

Where do we stand on changes to the IAppVeyorProvider interface? I was planning on moving the UploadArtifactSettings and UploadArtifactType (possibly also the MessageCategoryType and TestResultsType) into a new Cake.Common.Tools.AppVeyor namespace since, to my mind, they're inputs to the tool.

Problem being, they're also in the IAppVeyorProvider interface, so if I do, two things happen:

  1. We couple the provider to the tool. At the moment, it sort of is anyway, on account of the shelling out this PR is supposed to fix
  2. We change a public interface definition. I think in this instance this seems like not too much of a problem.

Thoughts on approach here? Just trying to avoid duplicating these settings and types..

devlead commented 6 years ago

@agc93 can we keep compatibility with existing scripts?

gep13 commented 6 years ago

@cake-build/cake-team going to bump this one again...

devlead commented 6 years ago

@cake-build/cake-team going to bump this one.

augustoproiete commented 2 years ago

@agc93 Bumping this one given we're working towards Cake v2.0 release so any breaking changes needed to IAppVeyorProvider might be easier to squeeze in now