Fix formatting #61
Fix formatting #61
Conversation
perlun
force-pushed
the
fix/indentation
branch
from
11e3f6f
to
9c23968
Compare
I had some spare time while waiting for a CEF/Chromium build (...) so I fixed this. As a side effect of the change, I also removed the debug building completely - it just takes time, we don't use the resulting artifacts anyway. So it makes more sense to _only_ build Release builds of this repo. Closes #60
perlun
force-pushed
the
fix/indentation
branch
from
9c23968
to
7b03e79
Compare
|
✅ Build cef-binary 62.0.0-CI31 completed (commit 05a546cad9 by @perlun) |
|
✅ Build cef-binary 62.0.0-CI34 completed (commit 05a546cad9 by @perlun) |
|
r? @merceyz |
appveyor.yml
Outdated
| @@ -5,9 +5,6 @@ version: 62.0.0-CI{build} | |||
|
|
|||
| shallow_clone: true | |||
|
|
|||
| # Start builds on tags only (GitHub and BitBucket) | |||
| skip_non_tags: true | |||
There was a problem hiding this comment.
Is there a reason you've removed this? How are you going to handle the version conflicts?
There was a problem hiding this comment.
Thanks for asking! Yes, there is a reason, it was discussed in #56.
(Short answer: We want to always run the AppVeyor builds on each PR, to ensure that the project still builds with each change. Ideally, the MyGet files should only be pushed whenever a tag had been added, though. I think this can be done, but I'm not 100% sure of how yet.)
Will revert this part of the change since, as you say, it is not a "formatting change" only.
build.ps1
Outdated
| Warn "Toolchain $Toolchain is not installed on your development machine, skipping build." | ||
| Return | ||
| } | ||
|
|
||
| Write-Diagnostic "Starting to build targeting toolchain $Toolchain" | ||
|
|
||
| Msvs "$Toolchain" 'Debug' 'x86' |
There was a problem hiding this comment.
From memory they're currently required for a debug build of CefSharp, are you sure this is wise?
There was a problem hiding this comment.
You are right here also. I thought it wasn't needed, but it very much is (since the /MTd etc flags need to match for the C++ projects.)
Will revert this also.
|
@amaitland Thanks for the constructive feedback. You are right, it's much better to segregate changes instead of messing it up like this, my bad. Please re-check at your convenience, the PR should be much better now. |
|
Thanks for the review & approval @amaitland, appreciated! I moved the discussion around the functional changes to #63, so we can get them incorporated also in a proper manner. |
I had some spare time while waiting for a CEF/Chromium build (...) so I fixed this. As a side effect of the change, I also removed the debug building completely - it just takes time, we don't use the resulting artifacts anyway. So it makes more sense to only build Release builds of this repo.
Closes #60