-
Notifications
You must be signed in to change notification settings - Fork 331
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
Source build support #1241
Source build support #1241
Conversation
Fake .NET desktop build environment if machine is non-windows. Fixes microsoft#1222.
Fix names for TestPlatform.nuspec and path to testhost.x86 binaries.
Use multitarget package instead.
Removed mono dependency checks. Fixes microsoft#1153.
fi | ||
|
||
# Copy TestHost for desktop targets | ||
local testhost=$packageDir/TestHost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we publish testhost here as we doing in build.ps1.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take this up in another PR.
@@ -39,7 +39,7 @@ def branch = GithubBranchName | |||
Utilities.setMachineAffinity(newJob, 'Windows_NT', 'latest-or-auto') | |||
break; | |||
|
|||
case "Ubuntu16.04": | |||
case "Ubuntu14.04": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making sure we run our CI in exact same configuration as Source Build repo. This will help catch issues earlier.
scripts/build.sh
Outdated
# Source build repo api | ||
# See https://github.com/dotnet/source-build/blob/dev/release/2.0/Documentation/RepoApi.md | ||
# | ||
DOTNETBUILDFROMSOURCE=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: DOTNET_BUILD_FROM_SOURCE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks.
;; | ||
-p) | ||
PROJECT_NAME_PATTERNS=$2 | ||
shift | ||
;; | ||
-dotnetbuildfromsource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't get it. We can't change the name of this arg, it is sent as is from RepoApi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change it to more readable DotNetBuildFromSource
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see Line 36. We are ensuring the script continues to run if user passes args in a different case.
@@ -80,8 +108,8 @@ TP_SRC_DIR="$TP_ROOT_DIR/src" | |||
export DOTNET_SKIP_FIRST_TIME_EXPERIENCE=1 | |||
# Dotnet build doesnt support --packages yet. See https://github.com/dotnet/cli/issues/2712 | |||
export NUGET_PACKAGES=$TP_PACKAGES_DIR | |||
DOTNET_CLI_VERSION="LATEST" | |||
DOTNET_RUNTIME_VERSION="LATEST" | |||
DOTNET_CLI_VERSION="2.1.0-preview1-007372" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While porting this to 15.3, make sure the vstest.console.runtimeconfig.json has compatible runtime with dotnet-cli servicing branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will ensure that in the Port PR to 15.3 branch. Current PR is for master.
scripts/build.sh
Outdated
|
||
log "Installing dotnet cli..." | ||
local start=$SECONDS | ||
if [[ $TP_USE_REPO_API ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks!
export FrameworkPathOverride=$TP_PACKAGES_DIR/microsoft.targetingpack.netframework.v4.6/1.0.1/lib/net46/ | ||
if [ -z "$PROJECT_NAME_PATTERNS" ] | ||
then | ||
$dotnet build $TPB_Solution --configuration $TPB_Configuration -v:minimal -p:Version=$TPB_Version -p:CIBuild=$TPB_CIBuild -p:LocalizedBuild=$TPB_LocalizedBuild || failed=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Script should exit on build failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See line 240.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have explicit check as line 179?
@@ -8,5 +8,7 @@ | |||
<!-- Don't include the output.dll and output.runtimeconfig.json files in nuget package --> | |||
<IncludeBuildOutput>false</IncludeBuildOutput> | |||
<GenerateRuntimeConfigurationFiles>false</GenerateRuntimeConfigurationFiles> | |||
<NoPackageAnalysis>true</NoPackageAnalysis> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not building this project, it is only required for dotnet pack
, hence disabled all these options.
@@ -17,6 +17,7 @@ | |||
<ItemGroup Condition=" '$(TargetFramework)' == 'net451' "> | |||
<ProjectReference Include="..\UnitTestProject\UnitTestProject.csproj" /> | |||
<Reference Include="System" /> | |||
<Reference Include="System.Runtime" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For unix, we are referring multitargeting pack to build desktop assemblies. It requires the dependencies to be explicitly called out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we make sure this for future commits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unix build will break if we add a dependency and not explicitly mention it in csproj. It will fail the PR :)
@@ -24,6 +24,14 @@ CI_BUILD=false | |||
VERBOSE=false | |||
PROJECT_NAME_PATTERNS= | |||
|
|||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update test.sh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dotnet cli(dotnet/<any_repo>) runs tests with build.cmd
, I think source-build
expects tests should run as part of build
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think source-build expects tests should run as part of build.
Didn't see any API related to running tests in the repo api docs: https://github.com/dotnet/source-build/blob/dev/release/2.0/Documentation/RepoApi.md. It only mentions build.cmd/sh
.
@@ -39,7 +39,7 @@ def branch = GithubBranchName | |||
Utilities.setMachineAffinity(newJob, 'Windows_NT', 'latest-or-auto') | |||
break; | |||
|
|||
case "Ubuntu16.04": | |||
case "Ubuntu14.04": | |||
// Define your build/test strings here. | |||
def buildString = """./build.sh -c ${configuration}""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to run tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not as part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dotnet/cli(dotnet/<any_repo>) runs tests with build.cmd
, I think source-build
expects tests should run as part of build
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, let's sync up. Here's the repo API doc I referred.
* Fix build issue for PlatformAbstractions on non windows environment. Fake .NET desktop build environment if machine is non-windows. Fixes microsoft#1222. * Use Ubuntu 14.04 since it is used in source builds. * Use exit code from build instead of log function. Fix names for TestPlatform.nuspec and path to testhost.x86 binaries. * Remove dependency on Mono for net4x builds. Use multitarget package instead. * Single build path for unix. Removed mono dependency checks. Fixes microsoft#1153. * Port vstest repo patches. Add support for repo API. https://github.com/dotnet/source-build/blob/dev/release/2.0/Documentation/RepoApi.md * Fix PR comments. * Use numeric comparisons instead of boolean in bash.
* Fix build issue for PlatformAbstractions on non windows environment. Fake .NET desktop build environment if machine is non-windows. Fixes #1222. * Use Ubuntu 14.04 since it is used in source builds. * Use exit code from build instead of log function. Fix names for TestPlatform.nuspec and path to testhost.x86 binaries. * Remove dependency on Mono for net4x builds. Use multitarget package instead. * Single build path for unix. Removed mono dependency checks. Fixes #1153. * Port vstest repo patches. Add support for repo API. https://github.com/dotnet/source-build/blob/dev/release/2.0/Documentation/RepoApi.md * Fix PR comments. * Use numeric comparisons instead of boolean in bash.
Allow vstest repository to build with
dotnet/source-build
.Validated these changes with symlink in
source-build
repo and built it successfully.Localization requires xlftool (which requires mono to be available). Will fix this outside current PR.