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

configure: handle path translation when importing settings from pkg-config #1160

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@jdtournier
Member

jdtournier commented Oct 6, 2017

Proposed solution to the issue raised here and here. Turns out there's a cygpath executable provided with MSYS2 (actually part of cygwin) to translate paths, so all this does is try to use that to translate include paths... Seems to allow MRtrix3 to build properly using the MinGW version of python.

Note this only translates header paths (i.e. starting with -I), not library search paths (which start with -L). Still, it builds fine with the default packages, so I've not done anything about it at this point. Worth bearing in mind for the future though...

@jdtournier jdtournier self-assigned this Oct 6, 2017

@jdtournier jdtournier requested review from Lestropie and bjeurissen Oct 6, 2017

@jdtournier

This comment has been minimized.

Show comment
Hide comment
@jdtournier

jdtournier Oct 11, 2017

Member

There's been a handful of reports of issues that seem related to this issue on the forum over the last couple of days - see this post and links therein. There is some urgency with this - can anyone review this and double-check that it does what it's supposed to...?

Note that testing this properly will probably need to be performed on an up to date installation of MSYS2 - you'll need to update that first to verify that there is indeed a problem on master, then checkout this branch and verify that this fix does what it's supposed to...

Member

jdtournier commented Oct 11, 2017

There's been a handful of reports of issues that seem related to this issue on the forum over the last couple of days - see this post and links therein. There is some urgency with this - can anyone review this and double-check that it does what it's supposed to...?

Note that testing this properly will probably need to be performed on an up to date installation of MSYS2 - you'll need to update that first to verify that there is indeed a problem on master, then checkout this branch and verify that this fix does what it's supposed to...

@Lestropie

This comment has been minimized.

Show comment
Hide comment
@Lestropie

Lestropie Oct 11, 2017

Member

On master:

  • MinGW python:
    • ./configure works
    • EIGEN_CFLAGS="-I/mingw64/include/eigen3" ./configure works
    • EIGEN_CFLAGS="-isystem /mingw64/include/eigen3" ./configure does not work
  • MSYS python:
    • ./configure works
    • EIGEN_CFLAGS="-I/mingw64/include/eigen3" ./configure works
    • EIGEN_CFLAGS="-isystem /mingw64/include/eigen3" ./configure works

On fix_mingw_python_path_handling:

  • MinGW python:
    • ./configure works
    • EIGEN_CFLAGS="-I/mingw64/include/eigen3" ./configure works
    • EIGEN_CFLAGS="-isystem /mingw64/include/eigen3" ./configure does not work
  • MSYS python:
    • ./configure works
    • EIGEN_CFLAGS="-I/mingw64/include/eigen3" ./configure works
    • EIGEN_CFLAGS="-isystem /mingw64/include/eigen3" ./configure works

So the only consistent fault I get is explicitly using the "-isystem" prefix on MinGW python.

I can't confirm whether or not your changes fix the issue. What I reported back here was using a manual eigen3 install rather than through pacman; and as such, I couldn't verify whether pkg-config was working, let alone cygpath. Now that I've deleted that and installed eigen3 through pacman, configure works out of the box for me on both branches, for both versions of python.

Also note the formation of PATH and eigen_cflags in config:
On master:

  • MinGW python:
    • PATH = r'C;\msys64\mingw64\bin;C;\msys64\home\Rob\src\mrtrix3\bin;C;\msys64\ ...
    • eigen_cflags = [ '-isystem', 'C:/msys64/mingw64/include/eigen3' ]
  • MSYS python:
    • PATH = r'/home/Rob/src/mrtrix3/bin:/home/Rob/src/robs_0.3/bin:/home/Rob/ ...
    • eigen_cflags = [ '-isystem', 'C:/msys64/mingw64/include/eigen3' ]

On fix_mingw_python_path_handling:

  • MinGW python:
    • PATH = r'C;\msys64\mingw64\bin;C;\msys64\home\Rob\src\mrtrix3\bin;C;\msys64\ ...
    • eigen_cflags = [ '-isystem', 'C:\msys64\mingw64\include\eigen3' ]
  • MSYS python:
    • PATH = r'/home/Rob/src/mrtrix3/bin:/home/Rob/src/robs_0.3/bin:/home/Rob/ ...
    • eigen_cflags = [ '-isystem', 'C:\msys64\mingw64\include\eigen3 ]`

So for me it looks like the two different python versions have different "native" representations of paths, as evident by the reported PATH, but all cygpath is doing in fix_mingw_python_path_handling (at least for me) is swapping forward slashes to backslashes.

Also: EIGEN_CFLAGS="-isystem C:/msys64/mingw64/include/eigen3" ./configure (i.e. manual Windows path rather than manual Unix path) works on either version of python.

So I have no idea how others are encountering the issue, assuming they have installed eigen3 via pacman.

Member

Lestropie commented Oct 11, 2017

On master:

  • MinGW python:
    • ./configure works
    • EIGEN_CFLAGS="-I/mingw64/include/eigen3" ./configure works
    • EIGEN_CFLAGS="-isystem /mingw64/include/eigen3" ./configure does not work
  • MSYS python:
    • ./configure works
    • EIGEN_CFLAGS="-I/mingw64/include/eigen3" ./configure works
    • EIGEN_CFLAGS="-isystem /mingw64/include/eigen3" ./configure works

On fix_mingw_python_path_handling:

  • MinGW python:
    • ./configure works
    • EIGEN_CFLAGS="-I/mingw64/include/eigen3" ./configure works
    • EIGEN_CFLAGS="-isystem /mingw64/include/eigen3" ./configure does not work
  • MSYS python:
    • ./configure works
    • EIGEN_CFLAGS="-I/mingw64/include/eigen3" ./configure works
    • EIGEN_CFLAGS="-isystem /mingw64/include/eigen3" ./configure works

So the only consistent fault I get is explicitly using the "-isystem" prefix on MinGW python.

I can't confirm whether or not your changes fix the issue. What I reported back here was using a manual eigen3 install rather than through pacman; and as such, I couldn't verify whether pkg-config was working, let alone cygpath. Now that I've deleted that and installed eigen3 through pacman, configure works out of the box for me on both branches, for both versions of python.

Also note the formation of PATH and eigen_cflags in config:
On master:

  • MinGW python:
    • PATH = r'C;\msys64\mingw64\bin;C;\msys64\home\Rob\src\mrtrix3\bin;C;\msys64\ ...
    • eigen_cflags = [ '-isystem', 'C:/msys64/mingw64/include/eigen3' ]
  • MSYS python:
    • PATH = r'/home/Rob/src/mrtrix3/bin:/home/Rob/src/robs_0.3/bin:/home/Rob/ ...
    • eigen_cflags = [ '-isystem', 'C:/msys64/mingw64/include/eigen3' ]

On fix_mingw_python_path_handling:

  • MinGW python:
    • PATH = r'C;\msys64\mingw64\bin;C;\msys64\home\Rob\src\mrtrix3\bin;C;\msys64\ ...
    • eigen_cflags = [ '-isystem', 'C:\msys64\mingw64\include\eigen3' ]
  • MSYS python:
    • PATH = r'/home/Rob/src/mrtrix3/bin:/home/Rob/src/robs_0.3/bin:/home/Rob/ ...
    • eigen_cflags = [ '-isystem', 'C:\msys64\mingw64\include\eigen3 ]`

So for me it looks like the two different python versions have different "native" representations of paths, as evident by the reported PATH, but all cygpath is doing in fix_mingw_python_path_handling (at least for me) is swapping forward slashes to backslashes.

Also: EIGEN_CFLAGS="-isystem C:/msys64/mingw64/include/eigen3" ./configure (i.e. manual Windows path rather than manual Unix path) works on either version of python.

So I have no idea how others are encountering the issue, assuming they have installed eigen3 via pacman.

@jdtournier

This comment has been minimized.

Show comment
Hide comment
@jdtournier

jdtournier Oct 25, 2017

Member

superseded by #1176 - closing.

Member

jdtournier commented Oct 25, 2017

superseded by #1176 - closing.

@jdtournier jdtournier closed this Oct 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment