Fix instances of incorrect if(ENV{...}) usage and ENV{PKG_CONFIG_PATH} not being reset

[LRFLEW]

I was experimenting with vcpkg in a way that required setting the environment variable PKG_CONFIG_PATH, but I was getting errors in vcpkg. As it turns out, there is a mistake in a few locations where the variable isn't tested correctly. if(ENV{PKG_CONFIG_PATH}) does not work (at least on my setup, macOS 10.14), and always results in the condition being evaluated as false/OFF. This is fixed by changing it to if(DEFINED ENV{PKG_CONFIG_PATH}). Performing a grep for if(ENV{ found one other instance, so I fixed that too.

While looking into this, I noticed that while (almost) all instances of modifying ENV{PKG_CONFIG_PATH} was setting a variable of the form BACKUP_ENV_PKG_CONFIG_PATH_*, not all of them were using it. Based on the variable's name, I assume this is a mistake and corrected all the instances of it (for this specific environment variable). This resulted in touching a few ports that used the environment variable. I'm not sure if it's appropriate to increment the Port-Version in this case (as it's more of a tooling update even in the ports), but I went ahead and incremented them, as that seems to be the protocol for this.

[JackBoosY]

cc @Neumann-A for review this PR because he is the pkgconfig author.

[Neumann-A]

we should define some helper macros for this kind of environment stuff

[LRFLEW]

I could use some clarification/direction for this. I see the tag "requires author response" was added. I'm assuming this means you want me to attempt turning the handling of ENV{PKG_CONFIG_PATH} into a macro(s). I can see this being reasonable to do with two macros (one to set the value, and one to revert it), and it can even be generalized to handle any ENV variable containing a list of paths (eg. ENV{PATH}).

@Neumann-A I haven't done much with macros in CMake. Are you talking about specifically using macro(), or would using function() be better here? Where exactly would I put this macro in the project? Would it be another .cmake file in scripts/cmake, or should it be added to an existing file somewhere? Also less pressingly, what would the naming convention be for these sorts of macros? I'm not sure I've seen anything like what you're talking about in the project already, so I don't have any references to go off of.

[LRFLEW]

Ok, I spent some time looking into the build error that CI found. I was able to confirm the build errors in my VM. After a lot of investigating, I finally figured out the source. As it turns out, one of the typos that this fixed resulted in a latent bug appearing. In vcpkg_configure_make.cmake, it is using this line to setup pkg-config a specific way (i.e. with a specific prefix):

set(ENV{PKG_CONFIG} "${PKGCONFIG} --define-variable=prefix=${_VCPKG_INSTALLED}${PATH_SUFFIX_${_buildtype}}")

This definition of the environment variable is never reset, which results in subsequent steps being affected by this line. This wasn't an issue before because vcpkg_find_acquire_program.cmake was incorrectly checking for the existence of this environment variable and was ignoring it. When I fixed the check in vcpkg_find_acquire_program.cmake, its behavior was being changed by calling vcpkg_configure_make() beforehand.

vcpkg_configure_make.cmake isn't the only file that's changing ENV{PKG_CONFIG}. Most notably, pretty much all the vcpkg_configure_* functions affect it. This means that fixing this properly will require updating quite a few files, so this is probably another good use case for a macro.

[JackBoosY]

@Neumann-A Do you have anyother questions?

[Neumann-A]

@JackBoosY No not really we should introduce something like vcpkg_internal_(set|prepend|append)_and_backup_env(envvar valuetoset config), vcpkg_internal_restore_env(envvar config) and vcpkg_internal_check_env_restored()

[LRFLEW]

Ok, a few quick comments / responses @Neumann-A

What exactly would vcpkg_internal_check_env_restored() be doing and what would its goal be. Is the idea that it checks if previously called functions / build scripts changed the ENV variables without reverting them? If so, wouldn't it need to be two functions, one to establish what the ENV variables should be and the other to check them?

The (set|prepend|append) set makes sense, but may not be entirely descriptive enough. The prepending that's going on here is specifically path-list prepending. There's behavior specific to path-lists going on, namely adding VCPKG_HOST_PATH_SEPARATOR between the added and existing values, but only when the existing value is non-empty. I was thinking the functions could be vcpkg_internal_set_and_backup_env and vcpkg_internal_prepend_path_and_backup_env. Appending could be added if there's any cases of it being used (which I haven't seen so far), and simple non-list prepending could be done with just set, as in vcpkg_internal_set_and_backup_env(VAR "prefix $ENV{VAR}" config).

Speaking of path-lists, there are a few instances with ENV{PKG_CONFIG_PATH} in particular where it's prepending multiple paths at once. Since they need to be separated by VCPKG_HOST_PATH_SEPARATOR, this makes the code for it fairly lengthy and hard to read. I was thinking it would be nice if the prepend_path function could take a list of paths to prepend and handle adding the separators itself. (this would require the path arguments to not be last, so it would be called like vcpkg_internal_prepend_path_and_backup_env(VAR config path1 path2 ...)). If it's a function, it would require converting from the CMake list separator (';') to the paths separator, which could cause issues if paths contain semicolons (though that's already a messy thing to be doing on Linux and macOS anyways). I think it should be possible to avoid this problem with a macro from what I understand, but I haven't worked with macros enough to know for sure.

Looking at the cases I found for this, I'm not sure how necessary the config argument is. There aren't any instances of the ENV variables being set in a nested context (from what I saw), and the current usage of this sort of config was setting it either to DEBUG or RELEASE, which doesn't really help in the case where called functions are affecting the ENV variable. Perhaps this should be removed or instead replaced with an automatic value that will result in less name collisions (such as the script filename if that's a thing that can be done).

Lastly, should these macros be strictly internal? This PR already found two instances in ports where the macros would be useful, and there's likely more for other ENV variables. It might be nice to allow the functions to be utilized from there.

[Neumann-A]

If so, wouldn't it need to be two functions, one to establish what the ENV variables should be and the other to check them?

vcpkg_internal_(set|prepend|append)_and_backup_env will append the added variable to an internal list
vcpkg_internal_restore_env will remove it from that list
vcpkg_internal_check_env_restored checks if the list is empty

he prepending that's going on here is specifically path-list prepending.

no it might not be path specific e.g. CFLAGS etc.. You would always use a set(ENV (${NEWVALUE}${SEP}${ENV}|${ENV}${SEP}${NEWVALUE})) here and let the caller decide how to do that. So the function requires an additional seperator argument (which will either be a space or the HOST_PATH_SEP).

Looking at the cases I found for this, I'm not sure how necessary the config argument is.

it is required there a cases where you want to first setup general configuration independent falgs and then configuration dependent flags. It also avoiding issues to to forgetting to correctly unset the previous configuration

[ras0219]

To preserve the (un)defined semantics of pkg_config_path, we should set/unset the backup variable.

Is there a way we can add tests for this to scripts/e2e_ports/ and scripts/azure-pipelines/end-to-end-tests.ps1? Maybe one could embed some .pc files and do testing about setting and unsetting PKG_CONFIG_PATH and calling various helpers.

[ras0219]

Suggested change

	if(BACKUP_ENV_PKG_CONFIG_PATH_RELEASE)
	if(DEFINED BACKUP_ENV_PKG_CONFIG_PATH_RELEASE)

[LRFLEW]

Just to add a point of clarification, this isn't as necessary as it is to add DEFINED when checking environment variables. That's because the form if(<variable>) checks if the variable is "defined to a value that is not a false constant" (if). This could be an issue if the environment variable was set to something like FALSE, so it probably should be changed everywhere it's used (or in the new macro), but it's a less pressing issue.

[ras0219]

Suggested change

	set(ENV{PKG_CONFIG_PATH} "${CURRENT_INSTALLED_DIR}/lib/pkgconfig")
	set(ENV{PKG_CONFIG_PATH} "${CURRENT_INSTALLED_DIR}/lib/pkgconfig")
	unset(BACKUP_ENV_PKG_CONFIG_PATH_RELEASE)

[LRFLEW]

The way this is written now, it's assumed that BACKUP_ENV_PKG_CONFIG_PATH_${_config} is unset going in. If it's not unset, then that's a larger bug. Rather than unsetting the variable it here, I think the broader solution is to check in vcpkg_internal_(set|prepend|append)_and_backup_env if it exists (or if it is in the list being kept for vcpkg_internal_check_env_restored) and throw an error if it is.

[JackBoosY]

@ras0219 Ping for review again.

[PhoebeHui]

@LRFLEW, could you resolve the conflicts?

[LRFLEW]

I've updated the code here, but the CI is showing regressions. The codebase has changed a lot since I first made this PR. One of the files was already fixed, so I removed that from this PR. The regressions are likely due to other ports having or being affected by the underlying bug. I'll take a closer look at this and try to get all the regressions fixed.

[JackBoosY]

This PR has been inactive for a long time, please reopen it if there is any progress.