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

AutotoolsToolchain: curate paths coming from tools.build:compiler_executables on Windows before defining CC, CXX etc #12194

Closed
wants to merge 38 commits into from

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Sep 24, 2022

Changelog: (Fix): In AutotoolsToolchain, if build machine is windows, curate paths coming from tools.build:compiler_executables before defining CC, CXX, CCAS, NVCC & FC env vars
Docs: conan-io/docs#2845

Scope of this PR has been simplified: #12194 (comment)

==========
Old logic description here but not valid anymore:

This PR tries to honor CC/CXX/LD/AR/NM/OBJDUMP/RANLIB/STRIP if they are set from profile -specially if a path was given-, fall back to good defaults for msvc if they are not set in profile, and convert them to a value compatible with autotools scripts & current subsysem (specially on Windows):

  • Users may set a full path to their compiler tools in these variables (specially when they have several compiler versions), and on Windows they likely set in their profile a "windows like" full path, so this path is converted to a path suitable for the subsystem used for autotools build.
  • If one of these variables is set with a path, but this path contains spaces, it falls back to a default value, because scripts generated by autotools can't handle spaces in paths of these variables (even if they are protected, path is quoted etc). A warning is displayed in this case.
    As a workaround on Windows, users may use short file names to refer to their compiler path if the long name contains spaces (PROGRA~1 instead of Program Files for example). I guess it might be possible for conan, if build machine is windows, to find the short file name (if it exists) of directories containing spaces, and do this conversion on the fly, but it's not implemented here (win32api.GetShortPathName from pywin32?).
  • If one of these variables is set but it's not a path (quite common also, like CC=cl), nothing special is done (except adding extra options like -nologo or eventually wrap with compile_wrapper & ar_wrapper as explained below).
  • If one of these variables is not set, it falls back to a good default if msvc, and nothing special is done for other compilers.
  • -nologo is added to CC, CXX & LD vars if msvc to avoid noisy messages during build.

All these values are stored in new AutotoolsToolchain attributes: cc, cxx, ld, ar, nm, objdump, ranlib, strip. If needed, these attributes can be overridden before calling generate() (where these values are injected in conanautotoolstoolchain script injected in build scope if they are different than the ones of VirtualBuildEnv).

This PR also adds two new arguments to AutotoolsToolchain constructor: compile_wrapper & ar_wrapper. They are None by default, but a recipe can fill them with path to a wrapper script like compile or ar-lib from automake (or sometimes source code of the library) to provide robust autotools compatibility to compilers like msvc.
If set, compile_wrapper wraps compiler calls (CC & CXX).
If set, ar_wrapper wraps archiver calls (AR).

A typical usage in an autotools based recipe would be:

    @property
    def _settings_build(self):
        return getattr(self, "settings_build", self.settings)

    @property
    def _user_info_build(self):
        return getattr(self, "user_info_build", self.deps_user_info)

    def build_requirements(self):
        if self._settings_build.os == "Windows":
            if is_msvc(self):
                # to get compile & ar-lib wrappers scripts if not provided in lib source code
                self.tool_requires("automake/1.16.5")
            if not self.conf.get("tools.microsoft.bash:path", default=False, check_type=bool):
                # to get a bash
                self.tool_requires("msys2/cci.latest")
            self.win_bash = True

    def generate(self):
        tc = AutotoolsToolchain(
            self,
            compile_wrapper = self._user_info_build["automake"].compile if is_msvc(self) else None,
            ar_wrapper = self._user_info_build["automake"].ar_lib if is_msvc(self) else None,
        )
        tc.generate()

Since AutotoolsToolchain may override CC, CXX etc, recipes using it must not call VirtualBuildEnv after AutotoolsToolchain, always before if they have to call it.

=======

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@SpaceIm SpaceIm force-pushed the autotoolstoolchain-msvc branch 2 times, most recently from 134a4a3 to 4a26229 Compare September 24, 2022 23:48
@SpaceIm SpaceIm changed the title AutotoolsToolchain: improve msvc handling AutotoolsToolchain: improve CC/CXX/LD env vars handling on Windows Sep 25, 2022
@SpaceIm SpaceIm changed the title AutotoolsToolchain: improve CC/CXX/LD env vars handling on Windows AutotoolsToolchain: improve CC/CXX/LD/NM env vars handling on Windows Sep 25, 2022
@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 25, 2022

@madebr @ericLemanissier if you have some time, I would appreciate feedbacks.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 25, 2022

I'm wondering whether AutotoolsToolchain could expose arguments to conditionally prefix these env vars with custom values in a recipe, like it's done in https://github.com/conan-io/conan-center-index/blob/1d04372ae1bead2e86a3f7183561361e9fcd9761/recipes/coin-cgl/all/conanfile.py#L90-L93

@SpaceIm SpaceIm changed the title AutotoolsToolchain: improve CC/CXX/LD/NM env vars handling on Windows AutotoolsToolchain: improve CC/CXX/LD/AR/NM env vars handling on Windows Sep 25, 2022
@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 25, 2022

Why not providing these scripts in conan (but GPL-2.0-or-later license)?

compile wrapper
#! /bin/sh
# Wrapper for compilers which do not understand '-c -o'.

scriptversion=2018-03-07.03; # UTC

# Copyright (C) 1999-2021 Free Software Foundation, Inc.
# Written by Tom Tromey <[email protected]>.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2, or (at your option)
# any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <https://www.gnu.org/licenses/>.

# As a special exception to the GNU General Public License, if you
# distribute this file as part of a program that contains a
# configuration script generated by Autoconf, you may include it under
# the same distribution terms that you use for the rest of that program.

# This file is maintained in Automake, please report
# bugs to <[email protected]> or send patches to
# <[email protected]>.

nl='
'

# We need space, tab and new line, in precisely that order.  Quoting is
# there to prevent tools from complaining about whitespace usage.
IFS=" ""	$nl"

file_conv=

# func_file_conv build_file lazy
# Convert a $build file to $host form and store it in $file
# Currently only supports Windows hosts. If the determined conversion
# type is listed in (the comma separated) LAZY, no conversion will
# take place.
func_file_conv ()
{
  file=$1
  case $file in
    / | /[!/]*) # absolute file, and not a UNC file
      if test -z "$file_conv"; then
	# lazily determine how to convert abs files
	case `uname -s` in
	  MINGW*)
	    file_conv=mingw
	    ;;
	  CYGWIN* | MSYS*)
	    file_conv=cygwin
	    ;;
	  *)
	    file_conv=wine
	    ;;
	esac
      fi
      case $file_conv/,$2, in
	*,$file_conv,*)
	  ;;
	mingw/*)
	  file=`cmd //C echo "$file " | sed -e 's/"\(.*\) " *$/\1/'`
	  ;;
	cygwin/* | msys/*)
	  file=`cygpath -m "$file" || echo "$file"`
	  ;;
	wine/*)
	  file=`winepath -w "$file" || echo "$file"`
	  ;;
      esac
      ;;
  esac
}

# func_cl_dashL linkdir
# Make cl look for libraries in LINKDIR
func_cl_dashL ()
{
  func_file_conv "$1"
  if test -z "$lib_path"; then
    lib_path=$file
  else
    lib_path="$lib_path;$file"
  fi
  linker_opts="$linker_opts -LIBPATH:$file"
}

# func_cl_dashl library
# Do a library search-path lookup for cl
func_cl_dashl ()
{
  lib=$1
  found=no
  save_IFS=$IFS
  IFS=';'
  for dir in $lib_path $LIB
  do
    IFS=$save_IFS
    if $shared && test -f "$dir/$lib.dll.lib"; then
      found=yes
      lib=$dir/$lib.dll.lib
      break
    fi
    if test -f "$dir/$lib.lib"; then
      found=yes
      lib=$dir/$lib.lib
      break
    fi
    if test -f "$dir/lib$lib.a"; then
      found=yes
      lib=$dir/lib$lib.a
      break
    fi
  done
  IFS=$save_IFS

  if test "$found" != yes; then
    lib=$lib.lib
  fi
}

# func_cl_wrapper cl arg...
# Adjust compile command to suit cl
func_cl_wrapper ()
{
  # Assume a capable shell
  lib_path=
  shared=:
  linker_opts=
  for arg
  do
    if test -n "$eat"; then
      eat=
    else
      case $1 in
	-o)
	  # configure might choose to run compile as 'compile cc -o foo foo.c'.
	  eat=1
	  case $2 in
	    *.o | *.[oO][bB][jJ])
	      func_file_conv "$2"
	      set x "$@" -Fo"$file"
	      shift
	      ;;
	    *)
	      func_file_conv "$2"
	      set x "$@" -Fe"$file"
	      shift
	      ;;
	  esac
	  ;;
	-I)
	  eat=1
	  func_file_conv "$2" mingw
	  set x "$@" -I"$file"
	  shift
	  ;;
	-I*)
	  func_file_conv "${1#-I}" mingw
	  set x "$@" -I"$file"
	  shift
	  ;;
	-l)
	  eat=1
	  func_cl_dashl "$2"
	  set x "$@" "$lib"
	  shift
	  ;;
	-l*)
	  func_cl_dashl "${1#-l}"
	  set x "$@" "$lib"
	  shift
	  ;;
	-L)
	  eat=1
	  func_cl_dashL "$2"
	  ;;
	-L*)
	  func_cl_dashL "${1#-L}"
	  ;;
	-static)
	  shared=false
	  ;;
	-Wl,*)
	  arg=${1#-Wl,}
	  save_ifs="$IFS"; IFS=','
	  for flag in $arg; do
	    IFS="$save_ifs"
	    linker_opts="$linker_opts $flag"
	  done
	  IFS="$save_ifs"
	  ;;
	-Xlinker)
	  eat=1
	  linker_opts="$linker_opts $2"
	  ;;
	-*)
	  set x "$@" "$1"
	  shift
	  ;;
	*.cc | *.CC | *.cxx | *.CXX | *.[cC]++)
	  func_file_conv "$1"
	  set x "$@" -Tp"$file"
	  shift
	  ;;
	*.c | *.cpp | *.CPP | *.lib | *.LIB | *.Lib | *.OBJ | *.obj | *.[oO])
	  func_file_conv "$1" mingw
	  set x "$@" "$file"
	  shift
	  ;;
	*)
	  set x "$@" "$1"
	  shift
	  ;;
      esac
    fi
    shift
  done
  if test -n "$linker_opts"; then
    linker_opts="-link$linker_opts"
  fi
  exec "$@" $linker_opts
  exit 1
}

eat=

case $1 in
  '')
     echo "$0: No command.  Try '$0 --help' for more information." 1>&2
     exit 1;
     ;;
  -h | --h*)
    cat <<\EOF
Usage: compile [--help] [--version] PROGRAM [ARGS]

Wrapper for compilers which do not understand '-c -o'.
Remove '-o dest.o' from ARGS, run PROGRAM with the remaining
arguments, and rename the output as expected.

If you are trying to build a whole package this is not the
right script to run: please start by reading the file 'INSTALL'.

Report bugs to <[email protected]>.
EOF
    exit $?
    ;;
  -v | --v*)
    echo "compile $scriptversion"
    exit $?
    ;;
  cl | *[/\\]cl | cl.exe | *[/\\]cl.exe | \
  icl | *[/\\]icl | icl.exe | *[/\\]icl.exe )
    func_cl_wrapper "$@"      # Doesn't return...
    ;;
esac

ofile=
cfile=

for arg
do
  if test -n "$eat"; then
    eat=
  else
    case $1 in
      -o)
	# configure might choose to run compile as 'compile cc -o foo foo.c'.
	# So we strip '-o arg' only if arg is an object.
	eat=1
	case $2 in
	  *.o | *.obj)
	    ofile=$2
	    ;;
	  *)
	    set x "$@" -o "$2"
	    shift
	    ;;
	esac
	;;
      *.c)
	cfile=$1
	set x "$@" "$1"
	shift
	;;
      *)
	set x "$@" "$1"
	shift
	;;
    esac
  fi
  shift
done

if test -z "$ofile" || test -z "$cfile"; then
  # If no '-o' option was seen then we might have been invoked from a
  # pattern rule where we don't need one.  That is ok -- this is a
  # normal compilation that the losing compiler can handle.  If no
  # '.c' file was seen then we are probably linking.  That is also
  # ok.
  exec "$@"
fi

# Name of file we expect compiler to create.
cofile=`echo "$cfile" | sed 's|^.*[\\/]||; s|^[a-zA-Z]:||; s/\.c$/.o/'`

# Create the lock directory.
# Note: use '[/\\:.-]' here to ensure that we don't use the same name
# that we are using for the .o file.  Also, base the name on the expected
# object file name, since that is what matters with a parallel build.
lockdir=`echo "$cofile" | sed -e 's|[/\\:.-]|_|g'`.d
while true; do
  if mkdir "$lockdir" >/dev/null 2>&1; then
    break
  fi
  sleep 1
done
# FIXME: race condition here if user kills between mkdir and trap.
trap "rmdir '$lockdir'; exit 1" 1 2 15

# Run the compile.
"$@"
ret=$?

if test -f "$cofile"; then
  test "$cofile" = "$ofile" || mv "$cofile" "$ofile"
elif test -f "${cofile}bj"; then
  test "${cofile}bj" = "$ofile" || mv "${cofile}bj" "$ofile"
fi

rmdir "$lockdir"
exit $ret

# Local Variables:
# mode: shell-script
# sh-indentation: 2
# eval: (add-hook 'before-save-hook 'time-stamp)
# time-stamp-start: "scriptversion="
# time-stamp-format: "%:y-%02m-%02d.%02H"
# time-stamp-time-zone: "UTC0"
# time-stamp-end: "; # UTC"
# End:

ar-lib wrapper
#! /bin/sh
# Wrapper for Microsoft lib.exe

me=ar-lib
scriptversion=2019-07-04.01; # UTC

# Copyright (C) 2010-2021 Free Software Foundation, Inc.
# Written by Peter Rosin <[email protected]>.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2, or (at your option)
# any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <https://www.gnu.org/licenses/>.

# As a special exception to the GNU General Public License, if you
# distribute this file as part of a program that contains a
# configuration script generated by Autoconf, you may include it under
# the same distribution terms that you use for the rest of that program.

# This file is maintained in Automake, please report
# bugs to <[email protected]> or send patches to
# <[email protected]>.


# func_error message
func_error ()
{
  echo "$me: $1" 1>&2
  exit 1
}

file_conv=

# func_file_conv build_file
# Convert a $build file to $host form and store it in $file
# Currently only supports Windows hosts.
func_file_conv ()
{
  file=$1
  case $file in
    / | /[!/]*) # absolute file, and not a UNC file
      if test -z "$file_conv"; then
	# lazily determine how to convert abs files
	case `uname -s` in
	  MINGW*)
	    file_conv=mingw
	    ;;
	  CYGWIN* | MSYS*)
	    file_conv=cygwin
	    ;;
	  *)
	    file_conv=wine
	    ;;
	esac
      fi
      case $file_conv in
	mingw)
	  file=`cmd //C echo "$file " | sed -e 's/"\(.*\) " *$/\1/'`
	  ;;
	cygwin | msys)
	  file=`cygpath -m "$file" || echo "$file"`
	  ;;
	wine)
	  file=`winepath -w "$file" || echo "$file"`
	  ;;
      esac
      ;;
  esac
}

# func_at_file at_file operation archive
# Iterate over all members in AT_FILE performing OPERATION on ARCHIVE
# for each of them.
# When interpreting the content of the @FILE, do NOT use func_file_conv,
# since the user would need to supply preconverted file names to
# binutils ar, at least for MinGW.
func_at_file ()
{
  operation=$2
  archive=$3
  at_file_contents=`cat "$1"`
  eval set x "$at_file_contents"
  shift

  for member
  do
    $AR -NOLOGO $operation:"$member" "$archive" || exit $?
  done
}

case $1 in
  '')
     func_error "no command.  Try '$0 --help' for more information."
     ;;
  -h | --h*)
    cat <<EOF
Usage: $me [--help] [--version] PROGRAM ACTION ARCHIVE [MEMBER...]

Members may be specified in a file named with @FILE.
EOF
    exit $?
    ;;
  -v | --v*)
    echo "$me, version $scriptversion"
    exit $?
    ;;
esac

if test $# -lt 3; then
  func_error "you must specify a program, an action and an archive"
fi

AR=$1
shift
while :
do
  if test $# -lt 2; then
    func_error "you must specify a program, an action and an archive"
  fi
  case $1 in
    -lib | -LIB \
    | -ltcg | -LTCG \
    | -machine* | -MACHINE* \
    | -subsystem* | -SUBSYSTEM* \
    | -verbose | -VERBOSE \
    | -wx* | -WX* )
      AR="$AR $1"
      shift
      ;;
    *)
      action=$1
      shift
      break
      ;;
  esac
done
orig_archive=$1
shift
func_file_conv "$orig_archive"
archive=$file

# strip leading dash in $action
action=${action#-}

delete=
extract=
list=
quick=
replace=
index=
create=

while test -n "$action"
do
  case $action in
    d*) delete=yes  ;;
    x*) extract=yes ;;
    t*) list=yes    ;;
    q*) quick=yes   ;;
    r*) replace=yes ;;
    s*) index=yes   ;;
    S*)             ;; # the index is always updated implicitly
    c*) create=yes  ;;
    u*)             ;; # TODO: don't ignore the update modifier
    v*)             ;; # TODO: don't ignore the verbose modifier
    *)
      func_error "unknown action specified"
      ;;
  esac
  action=${action#?}
done

case $delete$extract$list$quick$replace,$index in
  yes,* | ,yes)
    ;;
  yesyes*)
    func_error "more than one action specified"
    ;;
  *)
    func_error "no action specified"
    ;;
esac

if test -n "$delete"; then
  if test ! -f "$orig_archive"; then
    func_error "archive not found"
  fi
  for member
  do
    case $1 in
      @*)
        func_at_file "${1#@}" -REMOVE "$archive"
        ;;
      *)
        func_file_conv "$1"
        $AR -NOLOGO -REMOVE:"$file" "$archive" || exit $?
        ;;
    esac
  done

elif test -n "$extract"; then
  if test ! -f "$orig_archive"; then
    func_error "archive not found"
  fi
  if test $# -gt 0; then
    for member
    do
      case $1 in
        @*)
          func_at_file "${1#@}" -EXTRACT "$archive"
          ;;
        *)
          func_file_conv "$1"
          $AR -NOLOGO -EXTRACT:"$file" "$archive" || exit $?
          ;;
      esac
    done
  else
    $AR -NOLOGO -LIST "$archive" | tr -d '\r' | sed -e 's/\\/\\\\/g' \
      | while read member
        do
          $AR -NOLOGO -EXTRACT:"$member" "$archive" || exit $?
        done
  fi

elif test -n "$quick$replace"; then
  if test ! -f "$orig_archive"; then
    if test -z "$create"; then
      echo "$me: creating $orig_archive"
    fi
    orig_archive=
  else
    orig_archive=$archive
  fi

  for member
  do
    case $1 in
    @*)
      func_file_conv "${1#@}"
      set x "$@" "@$file"
      ;;
    *)
      func_file_conv "$1"
      set x "$@" "$file"
      ;;
    esac
    shift
    shift
  done

  if test -n "$orig_archive"; then
    $AR -NOLOGO -OUT:"$archive" "$orig_archive" "$@" || exit $?
  else
    $AR -NOLOGO -OUT:"$archive" "$@" || exit $?
  fi

elif test -n "$list"; then
  if test ! -f "$orig_archive"; then
    func_error "archive not found"
  fi
  $AR -NOLOGO -LIST "$archive" || exit $?
fi

@madebr
Copy link
Contributor

madebr commented Sep 25, 2022

I would do this another way and add cc/cxx/cpp/ld/ar/ranlib/nm attributes to the AutotoolsToolchain class.
When constructing a AutotoolsToolchain object, assign values to them.

This approach allows a recipe to override any variable without conan trying to be too smart (e.g. unwanted conversion of a path to unix path).

So allow this code:

def generate(self):
    tc = AutotoolsToolchain(self,
        compile_wrapper=self._user_info_build["automake"].compile if is_msvc(self) else None,
        ar_wrapper=self._user_info_build["automake"].ar_lib if is_msvc(self) else None,
    )
    assert tc.cc == "/path/to/compile cl"
    assert tc.cxx == "/path/to/compile cl"
    assert tc.ar == "/path/to/ar-lib lib"
    tc.cc = "my_awesome_compiler"
    assert tc.cc == "my_awesome_compiler"
    assert tc.cxx == "/path/to/compile cl"
    tc.generate()

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 25, 2022

The main problem I see with this approach is to delegate to recipes the responsibility to honor CC, CXX etc set as absolute paths in profiles, it might mean lot of boilerplate in each recipe while it would be better to delegate all of this to conan. If conan or recipe overrides CC with just cl while users have set this env var as an absolute path in their profile, it might not compile with expected msvc version.

@madebr
Copy link
Contributor

madebr commented Sep 25, 2022

If set, I would let the constructor initialize cc with the value of CC of the build host profile (after going through tools.unix_path).
This behavior also applies for a clang profile on any platform.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 25, 2022

But you lose compile or ar-lib wrapper injection, it shouldn't be handled by final users, it's an internal build detail of the recipe.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 25, 2022

Oh, I think I see what you mean, you just want to populate all these attributes during initialialization, expose them so that they may eventually be overridden by a recipe before tc.generate(), and after that they are injected in build scope during tc.generate()?

Just want to say that all these variables would be just None when related env var is not set in profile (except for msvc).

@SpaceIm SpaceIm changed the title AutotoolsToolchain: improve CC/CXX/LD/AR/NM/OBJDUMP/RANLIB/STRIP env vars handling on Windows AutotoolsToolchain: curate paths coming from tools.build:compiler_executables on Windows before defining CC, CXX etc Dec 2, 2022
@SpaceIm
Copy link
Contributor Author

SpaceIm commented Jan 18, 2023

@memsharded any chance to consider this PR? I don't know what to do, should I close it?

@memsharded
Copy link
Member

The main use case for this would be the msvc compiler in "Program Files" folders, as explained above.
But this would be dangerous as AutotoolsToolchain for msvc compiler is already creating and activating a vcvars environment. I am not sure that defining a different compiler location other than the one currently active in the current msvc environment is safe, but doesn't look so.

Together with the complexity of having to deal with win32 specific APIs, I am afraid that we cannot move this PR forward at this moment. Thanks very much for your contribution anyway.

Regarding some of the other discussions above, we definitely have in mind to develop some built-in helper that will automatically manage the definition of the environment, injection of wrappers, etc in AutotoolsToolchain, just we didn't have time with the 2.0, but it will be addressed later, and we think that could help to simplify many ConanCenter recipes.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 20, 2023

Could you at least convert paths from tools.build:compiler_executables with unix_path please? It's totally broken currently when build machine is Windows (and msvc is not the only compiler, you can use MinGW, or cross-build to Android with Android NDK, cross build to webassembly with emsdk etc, I have the feeling that conan is somewhat msvc centric when it comes to Windows, but there are other compilers). See conan-io/conan-center-index#17143

@SSE4
Copy link
Contributor

SSE4 commented Apr 20, 2023

why was it rejected?
if it's too risky or hard, it might be put into opt-in.

@yowidin
Copy link

yowidin commented Apr 26, 2023

@memsharded @madebr any feedback would be appreciated.

@memsharded
Copy link
Member

The use case, need, and solved problem is not evident enough to introduce the proposed complexity.

Quoting has worked well for lots of similar cases, are we fully sure that this is not just a matter of adding some quote somewhere, and avoid the added complexity?

For this to be considered, it would be necessary more evidence, well described detailed and real scenarios with reproducible cases (something that could also be a test in the test suite).

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 26, 2023

@memsharded I think that curating paths from tools.build:compiler_executables with unix_path() is not complex. Just drop the get_short_path_name() call which was the logic you were scary about.

It would become a one line change in AutotoolsToolchain:

-env.define(env_var, compilers_by_conf[comp])
+env.define(env_var, unix_path(self._conanfile, compilers_by_conf[comp]))

@memsharded
Copy link
Member

That change might be fair, but that is unrelated to this current PR.
For this kind of change to be moved forward it is better to create a new issue, not comments on PRs, specially when they are closed.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 26, 2023

This PR was exactly about that: curating CC, CXX etc on Windows. Why do you say it's unrelated?

@memsharded
Copy link
Member

The PR was about Windows shortening syntax with ~, not about the Windows subsystems paths, it is a very different problem, for different scope, that works in different cases.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 26, 2023

It was about curation of env vars in general. There is a call to unix_path in this PR. Nevermind.

@memsharded
Copy link
Member

This was not about env-vars in general but about the conf of tools.build:compiler_executables.

You are totally right about unix_path was there. The problem is that 99% of the code in this PR was the ctypes Windows path shortening and the possibility of dropping it and leaving theunix_path was done after the PR was closed, and that is very hard to track correctly. In the eyes of the reviewers, this PRs was not about unix_paths fixes for Windows subsystems, but something very different. Because all that ctypes code would be running always, not only for Windows subsystems.
So either the PR is re-opened, removing the rejected parts, or a new issue is created, or a new PR with just the unix_path is done (and hopefully with some tests, in general PRs without accompanying tests will not be merged, and the absence of tests make the PRs less likely to be moved forward, specially for something that is supposedly fixing a bug)

@yowidin
Copy link

yowidin commented Apr 26, 2023

@memsharded sorry for not reading the PR, I just assumed it was related since it was linked to my issue (can you also take a look at it for more context?).

My question is whether this kind of handling (namely wrapping compiler paths in unix_path) is something that could/should be implemented in the conan itself.

In my understanding with conan2, the conan is no longer "passive", in the sense that it just tagged the binaries before, but also tries to take care of the right flags for different platforms and settings, using different build tools. If that's correct, then this kind of fix shouldn't be out of place (in another PR and with all the tests of course).

@memsharded
Copy link
Member

Sure, @yowidin, thanks for the clarifications.

Yes, the purpose of the build system integrations like AutotoolsToolchain is to automate the most common cases, and Windows subsystems support is intended, it is definitely possible that there is a gap/bug in it and that it might require a fix.

I am going to spin off a new ticket for it.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 27, 2023

This was not about env-vars in general but about the conf of tools.build:compiler_executables.

This PR has been submitted before availability of tools.build:compiler_executables in conan client. In its original form it was catching CC, CXX etc env vars from profile and curate them so that it was robust to any path flavor set from profile (windows style, or spaces in paths) so that users don't have to care about underlying build system.

(and hopefully with some tests, in general PRs without accompanying tests will not be merged, and the absence of tests make the PRs less likely to be moved forward, specially for something that is supposedly fixing a bug)

I've found only 1 test of AutotoolsToolchain on Windows where autotools are called then configure then make, but it's disabled, so it's hard to write anything for this PR if the basic test without any CC/CXX set is not even covered :s

@memsharded
Copy link
Member

#13867 adding unix_paths for AutotoolsToolchain tools.build:compiler_executables, for Conan 2.0.5

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.

5 participants