Skip to content

Conversation

@directhex
Copy link
Contributor

This is based on #33292

@directhex
Copy link
Contributor 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
Contributor Author

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

@marek-safar
Copy link
Contributor

ios only

@directhex
Copy link
Contributor 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 directhex force-pushed the akoeplinger-add-ios branch from b79423c to d14e945 Compare March 11, 2020 17:47
@directhex
Copy link
Contributor 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 directhex force-pushed the akoeplinger-add-ios branch from d14e945 to b9fbf71 Compare March 12, 2020 12:35
@directhex
Copy link
Contributor Author

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

@directhex
Copy link
Contributor 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
Contributor Author

@akoeplinger not until darc starts working again!

@directhex directhex force-pushed the akoeplinger-add-ios branch from 2bb047b to fb12538 Compare March 13, 2020 18:56
@directhex directhex force-pushed the akoeplinger-add-ios branch from fb12538 to b2b8991 Compare March 13, 2020 19:31
@directhex
Copy link
Contributor 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
Contributor 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
Contributor 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
Contributor 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
Contributor 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