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

Move projects properties to Directory.Build.props #19325

Merged
merged 1 commit into from
Apr 11, 2021
Merged

Move projects properties to Directory.Build.props #19325

merged 1 commit into from
Apr 11, 2021

Conversation

Shkarlatov
Copy link
Contributor

Split *.csproj to local and shared properties.
It makes it easier to change global project properties and add a new conditions.

Documentation Customize your build

@penev92
Copy link
Member

penev92 commented Apr 7, 2021

Hey, thank you for your first contribution, it seems intriguing at a glance!

@pchote
Copy link
Member

pchote commented Apr 7, 2021

This looks really useful. The Windows .NET 5 build CI build is now consistently failing (i've rerun the workflow a couple of times to check), but i'm not sure if this is related to these changes or is some unrelated issue with the GH environment.

@Shkarlatov
Copy link
Contributor Author

This looks really useful. The Windows .NET 5 build CI build is now consistently failing (i've rerun the workflow a couple of times to check), but i'm not sure if this is related to these changes or is some unrelated issue with the GH environment.

May be add dotnet restore after dotnet nuget locals all --clear in

run: |
# Work around runtime failures on the GH Actions runner
dotnet nuget locals all --clear
.\make.ps1 check
dotnet build OpenRA.Test\OpenRA.Test.csproj -c Debug --nologo -p:TargetPlatform=win-x64
dotnet test bin\OpenRA.Test.dll --test-adapter-path:.

It github-env issue

@pchote
Copy link
Member

pchote commented Apr 7, 2021

Can you please try adding that in this PR?

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@pchote pchote mentioned this pull request Apr 7, 2021
OpenRA.WindowsLauncher/OpenRA.WindowsLauncher.csproj Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

Here are my notes from comparing removals and additions. Most of these are just information, but there are a few change requests in there.

Once the fixups are applied I will tag a devtest build so we can verify that packaged releases have not regressed.

Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
@pchote
Copy link
Member

pchote commented Apr 8, 2021

Can you please now squash the fixup commits? https://github.com/OpenRA/OpenRA/wiki/Contributing#updating-a-pull-request

@pchote
Copy link
Member

pchote commented Apr 8, 2021

I pushed a test build to https://github.com/pchote/OpenRA/releases/tag/devtest-20210408

The macOS, AppImage, and Windows 32bit builds appear to work as expected. The Windows 64bit build doesn't launch and doesn't even appear to crash, no output at all. Can anyone else repro?

@Shkarlatov
Copy link
Contributor Author

@pchote I tested builds. I cant build dotnet build -r win-x86 (win-x64 also). Without RID build is success. It is dotnet error dotnet/sdk#14281
Workaround: dotnet build -r win-x64 -p:ImportByWildcardBeforeSolution=false

@pchote
Copy link
Member

pchote commented Apr 8, 2021

That's odd. The x64 .NET 5 builds were working fine every time we've tested them before now - is something specific in this PR triggering the bug?

@Shkarlatov
Copy link
Contributor Author

@pchote How build x64? I download OpenRA-release-20210321.zip (src) , run dotnet build -r win-x86.

error NETSDK1134: Building a solution with a specific RuntimeIdentifier is not supported. If you would like to publish for a single RID, specifiy the RID at the individual project level instead.

C:\Program Files\dotnet\sdk\5.0.201\Current\SolutionFile\ImportAfter\Microsoft.NET.Sdk.Solution.targets(27,5): error NETSDK1134: сборка решения с заданным идентификатором RuntimeIdentifier не поддерживается. Если вы хотите выполнить публикацию для одного RID, укажите RID на уровне отдельного проекта. [D:\Downloads\OpenRA-release-20210321\OpenRA.sln]

Not sure if PR not contain problem.

@Shkarlatov
Copy link
Contributor Author

I found how build and packaging. msbuild -verbosity:m -nologo -t:Build -restore -p:Configuration=Release -p:TargetPlatform=win-x64 build success

@Shkarlatov
Copy link
Contributor Author

devenv /DebugExe "D:\Downloads\OpenRA-devtest-20210408-x64-winportable\Dune2000.exe"
Exception thrown at 0x00007FFEFEA6D759 (KernelBase.dll) in Dune2000.exe: 0x04242420 (parameters: 0x0000000031415927, 0x00007FFEBDDD0000, 0x0000006B2A97E3B0).
Exception thrown at 0x00007FFEFEA6D759 in Dune2000.exe: Microsoft C++ exception: HRException at memory location 0x0000006B2A97E778.
Exception thrown at 0x00007FFEFEA6D759 in Dune2000.exe: Microsoft C++ exception: [rethrow] at memory location 0x0000000000000000.
Exception thrown at 0x00007FFEFEA6D759 in Dune2000.exe: Microsoft C++ exception: HRException at memory location 0x0000006B2A97E778.
Exception thrown at 0x00007FFEFEA6D759 in Dune2000.exe: Microsoft C++ exception: EEFileLoadException at memory location 0x0000006B2A97A1C8.
Exception thrown at 0x00007FFEFEA6D759 in Dune2000.exe: Microsoft C++ exception: [rethrow] at memory location 0x0000000000000000.
Exception thrown at 0x00007FFEFEA6D759 in Dune2000.exe: Microsoft C++ exception: EEFileLoadException at memory location 0x0000006B2A97A1C8.
Failed to create CoreCLR, HRESULT: 0x80004005Failed to create CoreCLR, HRESULT: 0x80004005

Directory.Build.props Outdated Show resolved Hide resolved
@Shkarlatov Shkarlatov requested a review from pchote April 9, 2021 08:08
pchote
pchote previously approved these changes Apr 11, 2021
Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

LGTM and tested:

For some reason debugging through Rider is not working on my system (hangs during startup), but this is also a problem on bleed so does not appear to be related to this PR.

@pchote
Copy link
Member

pchote commented Apr 11, 2021

It would be good if we could get this merged sooner rather than later for the CI fix.

penev92
penev92 previously approved these changes Apr 11, 2021
Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

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

LGTM, just needs a few things to be addressed.

  • Also I notice UseVSHostingProcess properties have disappeared from 3 projects, but something tells me that's for the better?
  • And AutoGenerateBindingRedirects properties have also disappeared?

Directory.Build.props Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
Directory.Build.props Outdated Show resolved Hide resolved
@pchote
Copy link
Member

pchote commented Apr 11, 2021

@penev92
Copy link
Member

penev92 commented Apr 11, 2021

Thanks.

@pchote pchote dismissed stale reviews from penev92 and themself via f22b2cd April 11, 2021 22:33
@pchote
Copy link
Member

pchote commented Apr 11, 2021

I have force pushed fixups for @penev92's requests so we can merge this ASAP

@reaperrr reaperrr merged commit 2d05e10 into OpenRA:bleed Apr 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants