Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support '[package]' tag in PR CI and fix nightly build on macOS #5410

Merged
merged 5 commits into from
Nov 11, 2017
Merged

Support '[package]' tag in PR CI and fix nightly build on macOS #5410

merged 5 commits into from
Nov 11, 2017

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Nov 10, 2017 •
edited by daxian-dbw

  • Support '[package]' tag in PR CI to validate packaging changes in PR CI runs.
    When using '[package]' tag, regular CI tests will run and packaging steps will also run.
  • Fix nightly build on macOS.

@SteveL-MSFT
[daily]
85cbade 
added [daily] support
daxian-dbw
daxian-dbw reviewed Nov 10, 2017
View reviewed changes
tools/travis.ps1 Outdated
@@ -155,7 +155,7 @@ else
# Run a full build if the build was trigger via cron, api or the commit message contains `[Feature]`
$hasFeatureTag = $commitMessage -match '\[feature\]'
$hasRunFailingTestTag = $commitMessage -match '\[includeFailingTest\]'
$isDailyBuild = $env:TRAVIS_EVENT_TYPE -eq 'cron' -or $env:TRAVIS_EVENT_TYPE -eq 'api'
$isDailyBuild = $env:TRAVIS_EVENT_TYPE -eq 'cron' -or $env:TRAVIS_EVENT_TYPE -eq 'api' -or $commitMessage -match '\[daily\]'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's better to also create a variable $hasDailyTag, and use -or $hasDailyTag here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

TravisEz13
TravisEz13 reviewed Nov 10, 2017
View reviewed changes
tools/travis.ps1 Outdated
@@ -155,7 +155,7 @@ else
# Run a full build if the build was trigger via cron, api or the commit message contains `[Feature]`
$hasFeatureTag = $commitMessage -match '\[feature\]'
$hasRunFailingTestTag = $commitMessage -match '\[includeFailingTest\]'
$isDailyBuild = $env:TRAVIS_EVENT_TYPE -eq 'cron' -or $env:TRAVIS_EVENT_TYPE -eq 'api'
$isDailyBuild = $env:TRAVIS_EVENT_TYPE -eq 'cron' -or $env:TRAVIS_EVENT_TYPE -eq 'api' -or $commitMessage -match '\[daily\]'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update docs for new tag

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet. [Daily] tag is not working for AppVeyor PR CIs yet.
Should update docs after enabling the AppVeyor CI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could document that it only works in Travis-CI

daxian-dbw
daxian-dbw approved these changes Nov 10, 2017
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for one comment.

@daxian-dbw
Copy link
Member

@SteveL-MSFT Linux CI finished, please check if the package got uploaded.
For the broken macOS CI, #5400 is out trying to fix it.

@SteveL-MSFT SteveL-MSFT force-pushed the travis-mac branch 3 times, most recently from 664d81f to ec75cde Compare November 10, 2017 19:14
@daxian-dbw
Copy link
Member

daxian-dbw commented Nov 10, 2017 •
edited

For the [package] tag, I think it may be unnecessary to run all test like the full build. How about making [package] run tests like a regular PR CI but include the package related operation?
BTW, the regular AppVeyor PR CI always generates msi nupkg packages.

This comment doesn't block the PR. If we think that's the right way to go, we can submit a separate PR to do it.

@SteveL-MSFT
Copy link
Member Author

@daxian-dbw that makes sense since the intent is to test the packaging

@SteveL-MSFT
[package]
52c0462 
address PR feedback
daxian-dbw
daxian-dbw reviewed Nov 10, 2017
View reviewed changes
tools/appveyor.psm1 Outdated
# Run Daily tests
if($env:APPVEYOR_REPO_COMMIT_MESSAGE -match '\[feature\]')
if($env:APPVEYOR_REPO_COMMIT_MESSAGE -match '(\[feature\])|(\[package\])')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the change in AppVeyor. AppVeyor PR CI always generate packages.
Example: https://ci.appveyor.com/project/PowerShell/powershell/build/6.0.0-beta.9-6723

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, will remove

tools/travis.ps1
@@ -154,8 +154,9 @@ else

# Run a full build if the build was trigger via cron, api or the commit message contains `[Feature]`
$hasFeatureTag = $commitMessage -match '\[feature\]'
$hasPackageTag = $commitMessage -match '\[package\]'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line Start-PSBootstrap -Package:(-not $isPr) needs to be updated.
How about change to:

$hasPackageTag = $commitMessage -match '\[package\]'
$createPackages = -not $isPr -or $hasPackageTag
...
Start-PSBootstrap -Package:$createPackages
...
if ($createPackages) {
   ## create package

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will update

@daxian-dbw
Copy link
Member

Since the line Start-PSBootstrap -Package:(-not $isPr) is not updated, the build will fail to create the package. I have canceled the Travis CI because of this.

@SteveL-MSFT
[package]
948a72b 
address Dongbo's feedback
daxian-dbw
daxian-dbw approved these changes Nov 10, 2017
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@SteveL-MSFT
Copy link
Member Author

per @TravisEz13 Travis-CI doesn't upload the artifacts on PRs, so I'll have to wait til this is merged and a daily build kicks off to verify the actual package got uploaded, but it does appear to have been built

@daxian-dbw
Copy link
Member

It took forever for the macOS CI to start. I will merge this PR without waiting on the macOS because

  1. This PR will fix the current build break in macOS -- packaging is broken.
  2. The packages are not uploaded for PR CI runs. Merge CI run or daily run will trigger the uploading. @SteveL-MSFT cannot validate the generated arm32 package until this is merged.

@daxian-dbw daxian-dbw merged commit 54f34a0 into PowerShell:master Nov 11, 2017
@daxian-dbw daxian-dbw changed the title Fix nightly build on macOS Support '[package]' tag in PR CI and fix nightly build on macOS Nov 11, 2017
@SteveL-MSFT SteveL-MSFT deleted the travis-mac branch October 26, 2018 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@markekraus markekraus markekraus approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

Assignees

@daxian-dbw daxian-dbw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@SteveL-MSFT @daxian-dbw @markekraus @TravisEz13 @adityapatwardhan