Skip to content

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Nov 13, 2019

contributes to #1139.
With limited ability to commit to repose, I focus on passing parameters to the build.
Using patches when necessary.

cc: @joperator @jasonpugsley @am11

@wfurt wfurt requested review from crummel and omajid November 13, 2019 10:28
@wfurt wfurt self-assigned this Nov 13, 2019
UseHardlinksIfPossible="$(UseHardlink)" />

- <Exec Command="chmod +x $(TestHostRootPath)%(DotnetExe.Filename)%(DotnetExe.Extension)" Condition="'$(OS)' != 'Windows_NT'"/>
+ <Exec Command="chmod +x $(TestHostRootPath)%(DotnetExe.Filename)%(DotnetExe.Extension) || true" Condition="'$(OS)' != 'Windows_NT'"/>
Copy link
Member

Choose a reason for hiding this comment

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

Has someone from corefx looked at this change? We are (or were) hitting this on Fedora 32 and Arch Linux as well: #1310 (comment). Maybe this should be merged into corefx if it's the correct general fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that is my plan. but I'll do it after repo merge. I did testing and we don't need it at all any more. The dotnet is executable as it should on its own.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the build work without this change? I wouldn't expect test code to be getting hit in our normal build.

Copy link
Member

Choose a reason for hiding this comment

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

@crummel The build fails here for Fedora 32 and Arch Linux too: #1310 (comment). This code path is definitely invoked in source-build.

Copy link
Member Author

Choose a reason for hiding this comment

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

no, it does not build @crummel
It seems like it got moved to Arcade in master.
I'll take a look after repo merge is over.
cc: @safern

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW I think this is old time paranoia when we did not know how to set and preserve executable bits on Unix.

UseHardlinksIfPossible="$(UseHardlink)" />

- <Exec Command="chmod +x $(TestHostRootPath)%(DotnetExe.Filename)%(DotnetExe.Extension)" Condition="'$(OS)' != 'Windows_NT'"/>
+ <Exec Command="chmod +x $(TestHostRootPath)%(DotnetExe.Filename)%(DotnetExe.Extension) || true" Condition="'$(OS)' != 'Windows_NT'"/>
Copy link
Member

Choose a reason for hiding this comment

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

(In foreseeable future) Would it be possible to prepare TestHostRoot directory before executing the build.sh? How do the other platforms acquire it?

I had to figure out what constituents are required in this directory to execute corefx tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

This normally comes from TestHost and matching runtime entry. The problem is that if you get standard package (or if it lives in your nuget cache) it does not have platform reference and then dotnet is not restored and chmod fails.
At this point you may not even want to run tests but it breaks build it self. Conceptually I feel that is not correct.
To get to point where you can run tests on FreeBSD, I copy bootstrap dotnet over. (and related native libraries) You can pass --buildtests to top level script to build but not run tests @am11. This will go away and everything will work as other platforms once we have builds and packages flowing.

@wfurt
Copy link
Member Author

wfurt commented Nov 13, 2019

OSX failures seems like infrastructure issue

It was not possible to find any compatible framework version
  The specified framework 'Microsoft.NETCore.App', version '3.0.0' was not found.
    - Check application dependencies and target a framework version installed at:
        /Users/runner/.dotnet

@crummel crummel requested a review from dseefeld November 13, 2019 21:15
@crummel
Copy link
Contributor

crummel commented Nov 13, 2019

Yup, OSX failure is expected, I'm looking at it.

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.

One question on the test change patch, but looks good to me overall.

UseHardlinksIfPossible="$(UseHardlink)" />

- <Exec Command="chmod +x $(TestHostRootPath)%(DotnetExe.Filename)%(DotnetExe.Extension)" Condition="'$(OS)' != 'Windows_NT'"/>
+ <Exec Command="chmod +x $(TestHostRootPath)%(DotnetExe.Filename)%(DotnetExe.Extension) || true" Condition="'$(OS)' != 'Windows_NT'"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the build work without this change? I wouldn't expect test code to be getting hit in our normal build.

@crummel
Copy link
Contributor

crummel commented Nov 15, 2019

@wfurt Is this ready to go in?


<PropertyGroup>
<OverrideTargetRid>$(TargetRid)</OverrideTargetRid>
<OverrideTargetRid Condition="'$(TargetOS)' == 'FreeBSD'">freebsd-x64</OverrideTargetRid>
Copy link
Member

@omajid omajid Nov 15, 2019

Choose a reason for hiding this comment

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

Sorry if this is already obvious to everyone else, but what is the original value of TargetRid here that's being overridden?

Copy link
Contributor

Choose a reason for hiding this comment

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

We generate this at the very beginning of the build here:

<Target Name="WriteDynamicPropsToStaticPropsFiles">

That calls into our task which in turn is calling into core-setup code to get the RID the same way the product does. This will by default be a non-portable RID but it looks like FreeBSD, like Windows and OSX, always uses a portable RID - @wfurt will have to be the one to explain if you're interested in why that is.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, so this is just forcing a portable RID instead of the non-portable RID? That answers my question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we decided that last year but that change never made it to all repose. Also repos now are inconsistent so source-build is the glue to force same and expect RID.
One day we should make it simpler but for now we do want portable RID as all versions are binary and API compatible. (at least for parts we need)

@wfurt
Copy link
Member Author

wfurt commented Nov 15, 2019

it would be great if anybody can confirm that this works. But this should be good to go as it works at least in cases I tested and it does not break other platforms.

@crummel
Copy link
Contributor

crummel commented Nov 18, 2019

@dseefeld think we can go ahead with this? we can't verify that it remains working without CI but it doesn't seem to break anything else.

@dseefeld
Copy link
Contributor

@dseefeld think we can go ahead with this? we can't verify that it remains working without CI but it doesn't seem to break anything else.

Yes. I think we should merge this.

@crummel crummel merged commit feb9d91 into dotnet:release/3.0 Nov 18, 2019
crummel pushed a commit to crummel/dotnet_source-build that referenced this pull request Nov 21, 2019
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.

5 participants