[vcpkg_configure_make] Standardize OPTIONS list item handling

[dg0yt]

• What does your PR fix?

  This PR fixes cross-platform differences in vcpkg_configure_make by unifying the way how it invokes configure.
  Before this PR, the capabilites of passing spaces, quotes or bash variable references (e.g. $LIBS) were dependent on the platform:

  OPTIONS list item	Windows host	Other hosts	Remark
  LIBS=$LIBS -la -lb	fails (will be three parameters)	fails (literal $LIBS)
  LIBS=-la -lb	fails (will be two parameters)	works	libspatialite (hides detected LIBS)
  LIBS="$LIBS -la -lb"	works	fails (literal quotes and $LIBS)	libtasn1
  --with-a --with-b	works	works	starlink-ast

  With this PR, the capabilites are identical, and for spaces also consistent with vcpkg_configure_cmake:

  OPTIONS list item	All hosts	Remark
  LIBS=$LIBS -la -lb	works	To be used now, but with $LIBS at the end!
  LIBS=-la -lb	works	Should be used with $LIBS now.
  LIBS="$LIBS -la -lb"	fails (literal quotes)	To be transformed to the first form.
  --with-a --with-b	fails (will be one parameter)	To be split into individual list items.

  Note that "fails" is meant with regard to the particular use case. With this PR, quotes and spaces are generally passed literally.

  With the uniform invocation, it was possible to standardize and simplify other code in vcpkg_configure_make, such as handling ${prefix}.
  The PR also removes an unused EXTRA_QUOTES variable.
  A number of if(EXISTS ...) test are rectified to use pristine paths (no escaped spaces).

  Still unchanged: For maximum consistency, the BUILD_TRIPLET parameter should be changed to accept cmake lists, too.

• Which triplets are supported/not supported? Have you updated the CI baseline?

  all, no
  Tested manually on mingw (Windows and Linux hosts).

• Does your PR follow the maintainer guide?

  yes

• If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

  yes

[JackBoosY]

cc @Neumann-A for review this PR.

[dg0yt]

It is still a draft PR. I expected problems, but was too tired to figure out which ports to test. From previous CMake experience I know that dealing with double quotes, single quotes and escaping, cross-platform, can be quite tricky. An alternative could be writing and calling a bash script, and call this script uniformly. One could even look at the script in case of trouble.

[Neumann-A]

there is another way to pass env variables. E.g. CONFIGURE_ENVIRONMENT_VARIABLES and CONFIG_DEPENDENT_ENVIRONMENT

[dg0yt]

Thanks for the incoming review comments, despite the draft state. However it means you may raise issues which I planned to study in the course of the CI run, and adress before review. This PR is about lack of portability, and I can't cover this locally, quickly.

libtasn1 turned out to be the only port which already deals with LIBS (git grep 'LIBS=."' ports/*/portfile.cmake).
It uses this (but only for windows and mingw):

vcpkg/ports/libtasn1/portfile.cmake

Lines 21 to 25 in c801072

	if(VCPKG_TARGET_IS_WINDOWS OR VCPKG_TARGET_IS_MINGW)
	# $LIBS is an environment variable that vcpkg already pre-populated with some libraries.
	# We need to re-purpose it when passing LIBS option to make to avoid overriding the vcpkg's own list.
	set(EXTRA_OPTS "LIBS=\"$LIBS -lgettimeofday -lgetopt\"")
	else()

This fails now. (libtasn1 also has another use case: CFLAGS=....)

The problem with the above code is that it works for Windows because here arguments are substituted into a single string which is evaluated as a command line by bash. But for non-Windows targets, vcpkg_configure_make will pass the arguments literally to configure, i.e. with literal " and literal $. This means $LIBS wouldn't be expanded at all.

For more consistency and portability, we need to get completely rid of the branching around set(command "${base_cmd}" ....

In the end, the most portable solution might be to generate a parameterless bash script which handles setting up the environment and calling configure. This avoids the cross-platform and cmake quirks of argument passing and quoting in connection with variables, and it allows for inspection.

[dg0yt]

there is another way to pass env variables. E.g. CONFIGURE_ENVIRONMENT_VARIABLES and CONFIG_DEPENDENT_ENVIRONMENT

Thanks for this pointer. I will try that for libtasn1 in a separate PR. It is broken for mingw now, so it needs an update anyway.

[dg0yt]

there is another way to pass env variables. E.g. CONFIGURE_ENVIRONMENT_VARIABLES and CONFIG_DEPENDENT_ENVIRONMENT

Thanks for this pointer. I will try that for libtasn1 in a separate PR. It is broken for mingw now, so it needs an update anyway.

Unfortunately this won't work for variables which are already handled by vcpkg_configure_make. CONFIGURE_ENVIRONMENT_VARIABLES and CONFIG_DEPENDENT_ENVIRONMENT provide no means to mix detected cmake configuration with values provided by the portfile.

So in libtasn1, LIBS and CFLAGS are passed as parameters to configure, not as environment variables. AFAIK this takes precedence over environment variables of the same name.

For the Windows code path, the parameters are part of a command run in bash, with parameters expanded by bash at runtime. That's how libtasn1 manages to merge the LIBS environment variable from vcpkg_configure_make with the parameter it wants to add. And that's where the difficulties with spaces and quotes come from.

For the non-Windows code path, the parameters are passed directly to configure, without expansion in bash. This eliminates the need for quoting or escaping. But here, there is no way to combine with the values from vcpkg_configure_make with values supplied by the portfile.

The question remains: How to combine detected values with additional values, in a portable and clean way?

[Neumann-A]

The question remains: How to combine detected values with additional values, in a portable and clean way?

Add the additional values to the detected values. Or to be more precise make the additional values the detected values + the normally detected values.

[dg0yt]

Add the additional values to the detected values. Or to be more precise make the additional values the detected values + the normally detected values.

You mean to make detected values = additional values + normally detected values?

I considered this, too. But I couldn't see a clean and consistent way to do this. All examples I know use the OPTIONS of vcpkg_configure_make.

• Setting ENV{CFLAGS} might help for such flags, because CMake should pick it up in the detection step (prepending to CMAKE_C_FLAGS_INIT).
• For LIBS, I don't see that CMake is looking at an environment variable.
  EDIT: But vcpkg_configure_make appends to ENV{LIBS} if it is set before calling that function.

This doesn't look clean and consistent to me. Did I miss another approach or working example?

[dg0yt]

For the record, this is the current situation:

OPTIONS	Works on Windows	Works on non-Windows	Example
LIBS=$LIBS -la -lb	no (space)	no (dollar)
LIBS=-la -lb	no (space)	yes	libspatite (overwrites detected LIBS)
LIBS="$LIBS -la -lb"	yes	no (quotes, dollar)	libtasn1

where $LIBS refers to the environment variable LIBS at the time configure is invoked, i.e. the value set from vcpkg_configure_make based on detected CMake configuration.

[github-actions]

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for freexl but no changes to version or port version.
-- Version: 1.0.4#11
-- Old SHA: eb377ce039ce22eaec25f3bf81668c5d77bec086
-- New SHA: c7bbdaf0ed27ec1c61cd4302c976435ccbfb6dbf
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

[JackBoosY]

When building python3:x64-osx:

install: mkdir /Users/vagrant/Data/packages/python3_x64-osx/Users/vagrant/Data/installed/x64-osx/debug/lib: File exists
make: *** [altbininstall] Error 71
make: *** Waiting for unfinished jobs....

[dg0yt]

No more results from git grep '=""' | grep portfile ...

[JackBoosY]

Ping @strega-nil-ms for review again.

[BillyONeal]

Since this rebuilds a lot I just merged with master; will merge once build comes back

[BillyONeal]

/azp run

[BillyONeal]

Failure before was not related:

CMake Error at common/fw/CMakeLists.txt:61 (file):
  file DOWNLOAD HASH mismatch

    for file: [/mnt/vcpkg-ci/buildtrees/realsense2/x64-linux-rel/common/fw/L51X_FW_Image-1.5.8.1.bin]
      expected hash: [ab73e5bfc520c0aa0340cada4b3e317b8fd31a4d]
        actual hash: [da39a3ee5e6b4b0d3255bfef95601890afd80709]
             status: [56;"Failure when receiving data from the peer"]

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BillyONeal]

Thanks for your help!

[dg0yt]

Thanks for merging. Already needed for changes to the next port (libspatialite)...

[JackBoosY]

Thoese codes are not used, can you please double confirm that?

[dg0yt]

This code is used. Verified by CI. Verified once more manually by instrumenting the code and building libtasn1.