Skip to content

[release/2.1] Add portable builds to CI. #718

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
Merged
merged 5 commits into from
Sep 18, 2018
Merged

[release/2.1] Add portable builds to CI. #718

merged 5 commits into from
Sep 18, 2018

Conversation

crummel
Copy link
Contributor

@crummel crummel commented Aug 27, 2018 •
edited
Loading

Skip CI please.

@crummel crummel changed the title Add portable builds to CI. [release/2.1] Add portable builds to CI. Aug 27, 2018
@crummel crummel force-pushed the addPortableBuilds branch 3 times, most recently from 1b3aab1 to d1033eb Compare August 27, 2018 21:58
@crummel
Copy link
Contributor Author

crummel commented Aug 27, 2018

@dotnet-bot test CI please.

@crummel crummel force-pushed the addPortableBuilds branch from d1033eb to 36d39a6 Compare August 27, 2018 22:05
@crummel
Copy link
Contributor Author

crummel commented Aug 27, 2018

@dotnet-bot test CI please.

dagood
dagood approved these changes Aug 28, 2018
View reviewed changes
Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

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

LGTM, with a few maintenance/readability suggestions.

netci.groovy Outdated
def contextString = "${os} ${configuration}";
if (portable) {
contextString = "${os} ${configuration} Portable";
Copy link
Member

Choose a reason for hiding this comment

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

contextString += " Portable"; ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed throughout.

netci.groovy Outdated
{
def shortJobName = "${os}_${configuration}";
if (portable) {
shortJobName = "${os}_${configuration}_Portable";
Copy link
Member

Choose a reason for hiding this comment

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

+= here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

netci.groovy Outdated
// Per push, run all the jobs
["Ubuntu16.04", "Fedora24", "Debian8.4", "RHEL7.2", "Windows_NT", "CentOS7.1", "OSX10.12"].each { os ->
["Release", "Debug"].each { configuration ->
addPushJob(project, branch, os, configuration);
[true, false].each { portability ->
Copy link
Member

Choose a reason for hiding this comment

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

Stick with portable? Fits better with true, false, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, had started with these as strings and missed this one.

netci.groovy Outdated
def newJobName = Utilities.getFullJobName(project, "${os}_${configuration}", true);
def config = configuration;
if (portable) {
config = "${configuration}_Portable"
Copy link
Member

Choose a reason for hiding this comment

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

+= here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@crummel crummel self-assigned this Aug 28, 2018
@crummel
Copy link
Contributor Author

crummel commented Aug 28, 2018

One other thing I was considering was adding portable tarball or unshared builds - do we want these? They're a bit more work to add but I can refactor some.

@crummel crummel requested a review from dseefeld August 28, 2018 17:10
@crummel crummel force-pushed the addPortableBuilds branch from 18c9164 to b0afcd1 Compare August 28, 2018 17:10
@crummel
Copy link
Contributor Author

crummel commented Aug 28, 2018

@dotnet-bot test CI please

@dagood
Copy link
Member

dagood commented Aug 28, 2018

I can't think of a reason setting portable would fail during offline/tarball but not online, but on the other hand it seems like it would be good to know if it doesn't work. Maybe since offline/tarball involves doing an online build first, we could only do an offline build for portable=true on PRs?

@crummel
Copy link
Contributor Author

crummel commented Aug 28, 2018

Hmm, I'm not sure doing a non-portable online build followed by a portable offline build will work - wouldn't we possibly get some prebuilts with the wrong RID? I'll give this a try.

@crummel
Copy link
Contributor Author

crummel commented Aug 29, 2018

@dotnet-bot test CI please.

1 similar comment
@crummel
Copy link
Contributor Author

crummel commented Aug 30, 2018

@dotnet-bot test CI please.

@crummel crummel mentioned this pull request Sep 12, 2018
Closed
@crummel
Copy link
Contributor Author

crummel commented Sep 13, 2018

@dotnet-bot test CI please.

5 similar comments
@crummel
Copy link
Contributor Author

crummel commented Sep 13, 2018

@dotnet-bot test CI please.

@crummel
Copy link
Contributor Author

crummel commented Sep 14, 2018

@dotnet-bot test CI please.

@crummel
Copy link
Contributor Author

crummel commented Sep 15, 2018

@dotnet-bot test CI please.

@crummel
Copy link
Contributor Author

crummel commented Sep 16, 2018

@dotnet-bot test CI please.

@crummel
Copy link
Contributor Author

crummel commented Sep 17, 2018

@dotnet-bot test CI please.

@dagood
Copy link
Member

dagood commented Sep 17, 2018

When you're done with the CI testing (or merge this) could you let me know? I'd also like to test out #776 in practice.

@crummel
Copy link
Contributor Author

crummel commented Sep 17, 2018

I think this part of it is good to merge now, it's just that the new builds will fail until #717 is in, and that's essentially waiting on https://github.com/dotnet/core-eng/issues/4204 now. I'm fine with merging this now if those builds failing is okay for now.

@dagood
Copy link
Member

dagood commented Sep 17, 2018

I don't think we should merge this PR in that case, I just want to run the CI generation tests on my PR so you'd have to regenerate again on this PR if you want to test again. I'm going ahead with that, then.

@crummel
Copy link
Contributor Author

crummel commented Sep 17, 2018

Sounds good!

@crummel
Copy link
Contributor Author

crummel commented Sep 18, 2018

@dotnet-bot test CI please.

@crummel crummel merged commit 5d303ed into dotnet:release/2.1 Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dseefeld dseefeld dseefeld approved these changes

@dagood dagood dagood approved these changes

Assignees

@crummel crummel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@crummel @dagood @dseefeld