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

Switch back to patch instead of git apply. #470

Merged
merged 5 commits into from
Oct 10, 2019
Merged

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Oct 10, 2019

There were two problems with the fix for the ogre vendor patch:

  1. The downloaded ogre sources is not a git repository
  2. The fix used Unix line endings instead of DOS ones

It seems that newer versions of git do not allow you to call
'git apply' on a directory that is not a git repository. Worse,
'git apply' silently fails to apply the patch in that case
(actually, in all cases). So switch back to using patch, which
does properly throw errors and does properly work in a non-git
repository. Patch fails to apply mixed Unix-style and DOS-style
line endings, so also fix the patch up to use only DOS.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

Reviewers note: the final hunk of change to pragma-patch.diff seems like a no-op, but it subtly changes things so that we use CR/LF at the end of lines so Windows is happier with it.

This should fix all of the compile warnings we are getting from the Windows nightly jobs, like: https://ci.ros2.org/view/nightly/job/nightly_win_deb/1401/warnings43Result/

There were two problems with the fix for the ogre vendor patch:

1.  The downloaded ogre sources is not a git repository
2.  The fix used Unix line endings instead of DOS ones

It seems that newer versions of git do not allow you to call
'git apply' on a directory that is not a git repository.  Worse,
'git apply' silently fails to apply the patch in that case
(actually, in all cases).  So switch back to using patch, which
does properly throw errors and does properly work in a non-git
repository.  Patch fails to apply mixed Unix-style and DOS-style
line endings, so also fix the patch up to use only DOS.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

clalancette commented Oct 10, 2019

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Copy link
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Unfortunate approval.

@mjcarroll
Copy link
Member

I assume that this means that there will still be a need for an Administrator console on Windows for patch to succeed.

@clalancette
Copy link
Contributor Author

I assume that this means that there will still be a need for an Administrator console on Windows for patch to succeed.

Unfortunately, yes. git apply doesn't seem to be the tool we want; we should look around for something else, but that will have to be outside the scope of this PR.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@allenh1
Copy link
Contributor

allenh1 commented Oct 10, 2019

git apply doesn't seem to be the tool we want; we should look around for something else, but that will have to be outside the scope of this PR.

That's quite unfortunate.

I was told by @mfriedrich74 (offline) that it is possible to add an xml somewhere to remove the administrator restriction.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@mfriedrich74
Copy link

mfriedrich74 commented Oct 10, 2019

See https://docs.microsoft.com/en-us/windows/win32/sbscs/application-manifests

If the MS linker is used, you can use this:
https://docs.microsoft.com/en-us/cpp/build/reference/manifestuac-embeds-uac-information-in-manifest?view=vs-2019
/MANIFESTUAC:level=asInvoker

if you want manually want to write the application manifest (its linked as a resource of type RT_MANIFEST):

manifest.xml

<?xml version="1.0" encoding="UTF-8" standalone="yes"?> 
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">

  <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
    <security>
      <requestedPrivileges>
        <requestedExecutionLevel level="asInvoker" uiAccess="false"></requestedExecutionLevel>
      </requestedPrivileges>
    </security>
  </trustInfo>

</assembly>

app.rc

#include app_res.h

IDR_RT_APPMANIFEST      RT_MANIFEST             "manifest.xml"

app_res.h:

#define IDR_RT_APPMANIFEST              1

Then just compile app.rc and link the app.res into patch.exe.

@mfriedrich74
Copy link

If you can't (re)compile patch.exe, try creating a patch.exe.manifest file in the same folder as patch.exe.
patch.exe.manifest would need the same content as manifest.xml. I just do not know if this works only with executables create from MS compilers or with all.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

All right, I've now got it so that Windows doesn't show any warnings, builds are green, and I have an approval. I'll go ahead and merge this.

@mfriedrich74 Thanks for the tips. If we are going to move forward with patch.exe, then I think we'd have to do something like you suggest. Another potential option would be to use the 'patch.exe' that is shipped as part of the Windows Git installation, in C:\Program Files\Git\usr\bin\patch.exe; as far as I can tell, that doesn't require administrator privileges. In either case, we'd have to update our code (and potentially our installation instructions) to make sure we get what we expect. I'll leave that as an exercise for another day.

@clalancette clalancette merged commit e3461d7 into ros2 Oct 10, 2019
@clalancette clalancette deleted the fix-windows-warnings branch October 10, 2019 19:40
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.

None yet

4 participants