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

Enable POGO builds in MBv2 daily config, automatic NuGet generation and publication. Add build composition script. #863

Merged
merged 1 commit into from
Apr 29, 2016
Merged

Enable POGO builds in MBv2 daily config, automatic NuGet generation and publication. Add build composition script. #863

merged 1 commit into from
Apr 29, 2016

Conversation

dilijev
Copy link
Contributor

@dilijev dilijev commented Apr 25, 2016 •
edited
Loading

Includes progress on #85

This change is Reviewable

@dilijev
Copy link
Contributor Author

dilijev commented Apr 25, 2016

@digitalinfinity If you have a moment, please review?

ThomsonTan
ThomsonTan reviewed Apr 26, 2016
View reviewed changes
Build/scripts/pgo/pre_pgo.cmd
@echo off

if "%PogoConfig%"=="False" (
echo "---- Not a Pogo Config. Skipping step."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Double quotes are unnecesasry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming %PogoConfig% can only be controlled by the script, that's true, but to insulate in case of other possible values, I'd rather keep the quotes.

Copy link
Collaborator

@ThomsonTan ThomsonTan Apr 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't mean the double quotes around %PogoConfig%, the double quotes for echo is really unnecessary.

In reply to: 61509205 [](ancestors = 61509205)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but I think you could argue this is personal preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a grep for it, and all other instances of echo in cmd/bat files have no quotes. I'll make the change to be consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Emm, not just personal preference, the extra double quotes will be put into console output or log file, echo command doesn't interpret them.

In reply to: 61511010 [](ancestors = 61511010)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow I forgot about that. I've been using Git bash too much... bash's echo treats those quotes as parameter delimiters like is the case with other commands.

Anyway I'll remove them.

@dilijev
Copy link
Contributor Author

dilijev commented Apr 29, 2016

@Cellule Any review feedback or is this good to merge?

@dilijev
Enable POGO builds in MBv2 daily config, automatic NuGet generation a…
e59562f 
…nd publication. Add build composition script.
Cellule
Cellule reviewed Apr 29, 2016
View reviewed changes
Build/scripts/init_build.ps1
@@ -105,6 +106,13 @@ if (-not (Test-Path $flavorBuildIncompleteFile)) {
| Out-File $flavorBuildIncompleteFile -Encoding Ascii
}

$PogoConfig = "False"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should pass as argument which flavor we want to pogo.
This is okay for the time being, but we should consider changing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, we can follow up on desired changes for future iterations.

@Cellule
Copy link
Contributor

Cellule commented Apr 29, 2016

lgtm

@dilijev
Copy link
Contributor Author

dilijev commented Apr 29, 2016 •
edited
Loading

Thanks, @Cellule! Merging this.

@dilijev dilijev merged commit e59562f into chakra-core:release/1.2 Apr 29, 2016
dilijev added a commit that referenced this pull request Apr 29, 2016
@dilijev
[MERGE #863] Enable POGO builds in MBv2 daily config, automatic NuGet…
5ced209 
… generation and publication. Add build composition script.

Merge pull request #863 from dilijev:mbv2-nuget
Includes progress on #85
@dilijev dilijev deleted the mbv2-nuget branch April 29, 2016 23:47
dilijev added a commit that referenced this pull request Apr 30, 2016
@dilijev
[1.2>master] [MERGE #863] Enable POGO builds in MBv2 daily config, au…
e87031f 
…tomatic NuGet generation and publication. Add build composition script.

Merge pull request #863 from dilijev:mbv2-nuget
Includes progress on #85
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dilijev @Cellule @ThomsonTan @msftclas