Skip to content

[aot] Do not put quotes around wrapped path #16792

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.

Sign up for GitHub

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

Jump to bottom
Merged
monojenkins merged 1 commit into from
Sep 13, 2019
Merged

[aot] Do not put quotes around wrapped path #16792

monojenkins merged 1 commit into from
Sep 13, 2019

Conversation

@radekdoulik
Copy link
Contributor

@radekdoulik radekdoulik commented Sep 12, 2019 •
edited
Loading

Should fix: dotnet/android#3619

As it results in double quoting, which vanishes the effect and breaks
AOT for XA on Windows.

The actual error:

[aot-compiler stdout] Stripping the binary: ""C:\Program Files (x86)\Microsoft Visual Studio\2019\Stable\MSBuild\Xamarin\Android\ndk\arm-linux-androideabi-"strip" --strip-symbol=\$a --strip-symbol=\$d obj\Release\90\aot\armeabi-v7a\libaot-Xamarin.Android.Arch.Core.Common.dll.so.tmp
[aot-compiler stderr] '""C:\Program' is not recognized as an internal or external command,
[aot-compiler stderr] operable program or batch file.
[aot-compiler stderr] AOT of image C:\Users\peter\source\repos\App9\App9\App9.Android\obj\Release\90\android\assets\Xamarin.Android.Support.CoordinaterLayout.dll failed.

dellis1972
dellis1972 reviewed Sep 12, 2019
View reviewed changes
mono/mini/aot-compiler.c Outdated
Copy link
Contributor

@dellis1972 dellis1972 Sep 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@radekdoulik I don't think this will work.
The issue is wrap_path puts quotes around the tool_prefix like so

"c:\foo bar\ld-"

and then we append strip to it

"c:\foo bar\ld-"strip

Copy link
Contributor

@dellis1972 dellis1972 Sep 12, 2019

Choose a reason for hiding this comment

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

ignore this review, it appears that "c:\foo bar\ld-"strip does work on windows...

@radekdoulik
Copy link
Contributor Author

radekdoulik commented Sep 12, 2019

@monojenkins build failed

@lewurm
Copy link
Contributor

lewurm commented Sep 12, 2019

That's weird, isn't as and ld called in the same way? Why isn't it a problem there?

lewurm
lewurm suggested changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@lewurm lewurm left a comment

Choose a reason for hiding this comment

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

I think the proper fix would be to s/wrap_path(tool_prefix)/tool_prefix/

@radekdoulik
Copy link
Contributor Author

radekdoulik commented Sep 12, 2019

I think the proper fix would be to s/wrap_path(tool_prefix)/tool_prefix/

That would revert the 38ffd87. Unfortunately the information about the bug it was fixing is lost as the link https://bugzilla.xamarin.com/show_bug.cgi?id=34498 is not alive anymore.

The proposed fix should work OK and still contain the above mentioned fix.

@lewurm
Copy link
Contributor

lewurm commented Sep 12, 2019

It would only partially revert 38ffd87. wrap_path (tmp_outfile_name) looks fine.

We don't do wrap_path (tool_prefix) for the other calls tooling calls (as and ld) too, so I don't see why strip needs it there.

@radekdoulik
Copy link
Contributor Author

radekdoulik commented Sep 12, 2019

Me neither, because the bug information is lost.

OTOH we can assume that when the bug conditions would reappear, the as and ld would be affected too. So I guess it is pretty safe to assume the original issue was only affecting the tmp_outfile_name?

@radekdoulik
[aot] Do not wrap tool_prefix path when calling strip
abe4ac5 
Should fix: dotnet/android#3619

As it results in double quoting, which vanishes the effect and breaks
AOT for XA on Windows.

Let just quote the `strip` path as we do for `as` and `ld`.

The actual error:

    [aot-compiler stdout] Stripping the binary: ""C:\Program Files (x86)\Microsoft Visual Studio\2019\Stable\MSBuild\Xamarin\Android\ndk\arm-linux-androideabi-"strip" --strip-symbol=\$a --strip-symbol=\$d obj\Release\90\aot\armeabi-v7a\libaot-Xamarin.Android.Arch.Core.Common.dll.so.tmp
    [aot-compiler stderr] '""C:\Program' is not recognized as an internal or external command,
    [aot-compiler stderr] operable program or batch file.
    [aot-compiler stderr] AOT of image C:\Users\peter\source\repos\App9\App9\App9.Android\obj\Release\90\android\assets\Xamarin.Android.Support.CoordinaterL
  ayout.dll failed.
@radekdoulik radekdoulik force-pushed the pr-fix-aot-compiler-running-strip-for-android branch from 9c933e7 to abe4ac5 Compare September 12, 2019 12:11
@radekdoulik radekdoulik requested a review from lewurm September 12, 2019 12:37
@lewurm
Copy link
Contributor

lewurm commented Sep 12, 2019

The correct URL to access the bug info is https://bugzilla.internalx.com/show_bug.cgi?id=34498 (it was originally private, so you need to grant access via OAUTH).

From the context there, it seems like strip didn't make it in the first commit 3a85d50 and then on the follow up commit the mistake around tool_prefix happened.

@lewing
Copy link
Member

lewing commented Sep 12, 2019

@monojenkins build Windows x64

@radekdoulik
Copy link
Contributor Author

radekdoulik commented Sep 13, 2019

@monojenkins build failed

@radekdoulik
Copy link
Contributor Author

radekdoulik commented Sep 13, 2019

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 13, 2019

Commenter does not have sufficient privileges for PR 16792 in repo mono/mono

@radekdoulik
Copy link
Contributor Author

radekdoulik commented Sep 13, 2019

the failing tests are not required, so hopefully it is OK and we don't need them to re-run

@dellis1972
Copy link
Contributor

dellis1972 commented Sep 13, 2019

@radekdoulik which branches does this need to be on, any idea?

@radekdoulik
Copy link
Contributor Author

radekdoulik commented Sep 13, 2019

2019-08 for sure. @pjcollins might know if we need it in any earlier branch. I think the bug surfaced now because we recently changed the binutil tools location in XA as we redistribute them. so hopefully older versions are not affected.

@lewurm
Copy link
Contributor

lewurm commented Sep 13, 2019

@monojenkins squash

@lewurm
Copy link
Contributor

lewurm commented Sep 13, 2019

@monojenkins backport 2019-08

@monojenkins monojenkins mentioned this pull request Sep 13, 2019
Merged
@monojenkins monojenkins merged commit 131b3a9 into mono:master Sep 13, 2019
@pjcollins
Copy link
Contributor

pjcollins commented Sep 13, 2019

I believe master and 2019-08 are the only affected, this will be needed for our d16-4 branch and later.

monojenkins added a commit that referenced this pull request Sep 13, 2019
@monojenkins
[aot] Do not wrap tool_prefix path when calling strip (#16820)
20308e6 
[2019-08] [aot] Do not put quotes around wrapped path

Should fix: dotnet/android#3619

As it results in double quoting, which vanishes the effect and breaks
AOT for XA on Windows.

The actual error:

    [aot-compiler stdout] Stripping the binary: ""C:\Program Files (x86)\Microsoft Visual Studio\2019\Stable\MSBuild\Xamarin\Android\ndk\arm-linux-androideabi-"strip" --strip-symbol=\$a --strip-symbol=\$d obj\Release\90\aot\armeabi-v7a\libaot-Xamarin.Android.Arch.Core.Common.dll.so.tmp
    [aot-compiler stderr] '""C:\Program' is not recognized as an internal or external command,
    [aot-compiler stderr] operable program or batch file.
    [aot-compiler stderr] AOT of image C:\Users\peter\source\repos\App9\App9\App9.Android\obj\Release\90\android\assets\Xamarin.Android.Support.CoordinaterLayout.dll failed.


Backport of #16792.

/cc @lewurm @radekdoulik
ManickaP pushed a commit to ManickaP/runtime that referenced this pull request Jan 20, 2020
@radekdoulik @monojenkins
[aot] Do not wrap tool_prefix path when calling strip (mono/mono#16792)
2f86e6c 
[aot] Do not put quotes around wrapped path

Should fix: dotnet/android#3619

As it results in double quoting, which vanishes the effect and breaks
AOT for XA on Windows.

The actual error:

    [aot-compiler stdout] Stripping the binary: ""C:\Program Files (x86)\Microsoft Visual Studio\2019\Stable\MSBuild\Xamarin\Android\ndk\arm-linux-androideabi-"strip" --strip-symbol=\$a --strip-symbol=\$d obj\Release\90\aot\armeabi-v7a\libaot-Xamarin.Android.Arch.Core.Common.dll.so.tmp
    [aot-compiler stderr] '""C:\Program' is not recognized as an internal or external command,
    [aot-compiler stderr] operable program or batch file.
    [aot-compiler stderr] AOT of image C:\Users\peter\source\repos\App9\App9\App9.Android\obj\Release\90\android\assets\Xamarin.Android.Support.CoordinaterLayout.dll failed.



Commit migrated from mono/mono@131b3a9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lambdageek lambdageek Awaiting requested review from lambdageek

@SamMonoRT SamMonoRT Awaiting requested review from SamMonoRT

@vargaz vargaz Awaiting requested review from vargaz

2 more reviewers

@lewurm lewurm lewurm approved these changes

@dellis1972 dellis1972 dellis1972 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AOT compilation now fails when invoking strip on Windows

6 participants

@radekdoulik @lewurm @lewing @dellis1972 @pjcollins @monojenkins