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

Add AzDO builds for iOS #33424

Merged
merged 16 commits into from
Mar 17, 2020
Merged

Conversation

directhex
Copy link
Member

This is based on #33292

@directhex
Copy link
Member Author

Dang I meant to make this a draft

@directhex directhex force-pushed the akoeplinger-add-ios branch 2 times, most recently from 667a43a to d82bb8f Compare March 10, 2020 16:22
@directhex
Copy link
Member Author

@marek-safar on iOS only, or on OSX as well?

@marek-safar
Copy link
Contributor

ios only

@directhex
Copy link
Member Author

Well, this seems to be doing the expected thing?

@marek-safar
Copy link
Contributor

You need to hook up libraries build as well

@directhex
Copy link
Member Author

@marek-safar ok, looks like I wired in iOS Libraries. This still needs rebasing against the final version of #33292 but the AzDO logic seems sound

@directhex
Copy link
Member Author

OK, I rebuilt the branch against master post-merge. Let's see what happens

@directhex
Copy link
Member Author

Yeah that's looking OK. Ready for review I reckon.

@steveisok steveisok requested a review from a team March 12, 2020 14:04
@akoeplinger
Copy link
Member

@directhex can you try doing an official build to make sure it works there too?

@directhex
Copy link
Member Author

@akoeplinger not until darc starts working again!

@directhex
Copy link
Member Author

@safern I can't reproduce the failure in the official build - https://dev.azure.com/dnceng/7ea9116e-9fac-403d-b258-b31fcf1bb293/_apis/build/builds/558857/logs/261

Suggestions?

@safern
Copy link
Member

safern commented Mar 13, 2020

it seems like you're somehow missing an space in between iOS and -stripSymbols? I see this is how build-native.sh is being called.

/Users/runner/runners/2.165.0/work/1/s/src/libraries/Native/build-native.sh x64 Release outconfig netcoreapp5.0-iOS-Release-x64 -os iOS-stripSymbols

@directhex
Copy link
Member Author

@safern I've figured out why that's happening, and could use input from @Anipik & @ViktorHofer to understand the right way to fix it (i.e. what the expected behaviour is supposed to be)

In eng/pipelines/libraries/base-job.yml, the variable _stripSymbolsArg is conditionally defined as -stripSymbols on non-Windows. It is then passed as a parameter to ./libraries.sh, i.e. I get a run of ./libraries.sh -build -configuration Release -ci -arch x64 -stripSymbols -os iOS /p:OfficialBuildId=20200316.5 /p:RuntimeArtifactsPath=/Users/runner/runners/2.165.0/work/1/s/artifacts/transport/coreclr - however, all variables in AzDO are exported as environment... so in src/libraries/Native/build-native.proj, _stripSymbolsArg is redefined as -stripsymbols (note the leading space) only when BuildNativeStripSymbols is true, and TargetOS is NOT wasm (or iOS in the specific case of this PR).

As a result, during command line construction in build-native.proj, $(_BuildNativeArgs)$(_ProcessCountArg)$(_StripSymbolsArg) has a leading space on _StripSymbolsArg only when TargetOS is NOT iOS/wasm, or when reproducing locally (when you aren't going to be exporting the no-space value from the YAML)

Is the expectation that -stripSymbols should be EMPTY on iOS, or not? (if not, why is it in the conditional in build-native.proj)

@safern
Copy link
Member

safern commented Mar 16, 2020

Arrgg I hate when that happens 😠

So I would leave -stripSymbols definition in the yml with the current conditions as is and without any quotes or spaces. What I would recommend doing is just rename the yml variable to be, _stripSymbolsArgYaml and add a comment on why the name and to not rename.

Yes based on what @akoeplinger the expectation is that -stripSymbolsArg is empty for iOS. So I would also condition it when setting it in the yml to when osGroup != iOS.

@directhex
Copy link
Member Author

I guess same question to @akoeplinger, who added ios to the same conditional as wasm - should symbol stripping happen or not on iOS?

@directhex
Copy link
Member Author

OK, 'Mismatched MVIDs in ibc data' means nothing to me

https://dev.azure.com/dnceng/7ea9116e-9fac-403d-b258-b31fcf1bb293/_apis/build/builds/561059/logs/288

@akoeplinger
Copy link
Member

should symbol stripping happen or not on iOS?

I didn't really know which is why I just copied webassembly for now 😄

@directhex
Copy link
Member Author

I think the answer is "no" because the symbol stripping depends on objcopy, which isn't available in this scenario

@akoeplinger
Copy link
Member

Failures are unrelated, merging.

@akoeplinger akoeplinger merged commit 65a1d2c into dotnet:master Mar 17, 2020
jashook pushed a commit that referenced this pull request Mar 19, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants