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.

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

Fix cli-deps-satellites on windows to not sign the satellites during source-build, since we are using a dummy RoslynTools package. #148

Merged
merged 4 commits into from
Sep 19, 2017

Conversation

eerhardt
Copy link
Member

@eerhardt
Copy link
Member Author

@mmitche - do you know why the Windows CI job isn't running? I updated the netci.groovy in my previous commit - 0e41e37#diff-2d212ad84506951c8f928e21aed24580R48.

But it doesn't appear to have picked it up.

@mmitche
Copy link
Member

mmitche commented Sep 15, 2017

@eerhardt The webhook for pushes was missing. Fixed that and ran the generator manually.

@eerhardt
Copy link
Member Author

Thanks!

@eerhardt
Copy link
Member Author

Let me know what you guys think about the pattern here - '$(DotNetBuildType)' == 'Source'. Basically, this patch could get PR'd into the sub-repo, and it should be able to work long term with no changes to source-bulid.

Obviously setting the parameter through an EnvironmentVariable will change, but that is prior art that I'm not changing here.

…source-build, since we are using a dummy RoslynTools package.
@eerhardt eerhardt closed this Sep 18, 2017
@eerhardt eerhardt reopened this Sep 18, 2017
Copy link
Contributor

@crummel crummel left a comment

Choose a reason for hiding this comment

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

I like this pattern - it works toward getting patches included in the sub-repos and leaves the BuildType open to expansion if we ever need it.

@weshaggard
Copy link
Member

Let me know what you guys think about the pattern here - '$(DotNetBuildType)' == 'Source'. Basically, this patch could get PR'd into the sub-repo, and it should be able to work long term with no changes to source-bulid.

Is there a reason you chose this pattern over just '$(SourceBuild)' == 'true'? While I don't mind what you have it leads me to wonder what other build types there would be and it also lends itself to case-sensitive type problems and renaming problems etc. I personally would prefer a boolean option for this instead. We also switched to that model for OS checks (https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/Build.Common.props#L29) and msbuild type (https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/Build.Common.props#L31) which to me make it easier.

@eerhardt
Copy link
Member Author

Is there a reason you chose this pattern over just '$(SourceBuild)' == 'true'? While I don't mind what you have it leads me to wonder what other build types there would be

@jaredpar had suggested this offhand last week and it made sense to me. I can imagine we would also have a "MicrosoftOfficial" build type as well, and individual repos can adjust their build logic as necessary.

and it also lends itself to case-sensitive type problems and renaming problems etc

MSBuild == comparisons are case-insensitive by default.

What approach wouldn't have renaming problems?

I personally would prefer a boolean option for this instead.

I'd like to not have an explosion of properties that source-build is going to pass into the repos. I could go either way, and would like to hear @jaredpar's opinion on it, since he gave me the idea.

One other topic is the usage of a prefix DotNet for all properties passed from source-build to the repo.
In my mind, it makes sense to separate these properties since they will be used in such a large number of repos.

@weshaggard
Copy link
Member

MSBuild == comparisons are case-insensitive by default.

I guess I've been bitten too many times by case sensitive compares, but I think you are correct here but I know case still matters for file paths in msbuild.

One other topic is the usage of a prefix DotNet for all properties passed from source-build to the repo.
In my mind, it makes sense to separate these properties since they will be used in such a large number of repos.

I like the idea of a prefix as well and "DotNet" is fine.

What approach wouldn't have renaming problems?

True, I guess like like to think of the property names as more strongly typed then a string value being compared, but in msbuild that isn't the case as it will let you use any property name without complaining.

I'd like to not have an explosion of properties that source-build is going to pass into the repos.

I don't want there to be an explosion either but for this case I don't expect other options to passed, but I guess only time will tell for certain.

@eerhardt
Copy link
Member Author

eerhardt commented Sep 18, 2017

I talked to Wes more about the above issue. He's convinced me that it is easier to reason about bool properties. One example: when MSBuild tried adding a new value to $(OS), it was backed out as a breaking change - See dotnet/msbuild#539.

Also, fix nuget-client build to not build for net46 during source-build, since there is no MSBuild ref assemblies for net46 during source-build.
@weshaggard
Copy link
Member

weshaggard commented Sep 18, 2017

FWIW it has become clear to me that we are going to that there is going to be at least two modes that the source-build repo is going to have to be built in. One is the build everything from source mode (aka distro tarball builds) and the other is you can use pre-existing packages mode (aka official binary shipping product). Hopefully for the latter we don't need to pass any flags and it will just be the normal build but I wanted to at least call out these modes.

@weshaggard
Copy link
Member

To complete my thought in the last comment I think it may make it clearer if we call this property DotNet_BuildFromSource or DotNet_BuildOnlyFromSource.

@eerhardt
Copy link
Member Author

Renamed to DotNet_BuildFromSource.

@eerhardt
Copy link
Member Author

Merging to continue progress.

@eerhardt eerhardt merged commit f6b0be4 into dotnet:master Sep 19, 2017
@eerhardt eerhardt deleted the FixWindows branch September 19, 2017 17:47
@weshaggard weshaggard mentioned this pull request Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants