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

conda_build/windows.py: avoid trailing semicolon in %MSYS2_ARG_CONV_EXCL% #1651

Merged
merged 1 commit into from Jan 20, 2017

Conversation

pkgw
Copy link
Contributor

@pkgw pkgw commented Jan 11, 2017

The previous method of setting the %MSYS2_ARG_CONV_EXCL% environment variable tried to preserve any preexisting value of the variable. Not only does this make the build process depend on the initial environment more, it generally leads to a trailing semicolon in the variable value. This seems to cause MSYS2 to parse the variable as having an empty value that prevents all argument conversions, as investigated in the conda-forge cairo package: see here.

I do not know what fallout this change will have for existing build scripts, but the current behavior cannot be what was intended.

…XCL%

The previous method of setting the %MSYS2_ARG_CONV_EXCL% environment variable
tried to preserve any preexisting value of the variable. Not only does this
make the build process depend on the initial environment more, it generally
leads to a trailing semicolon in the variable value. This seems to cause MSYS2
to parse the variable as having an empty value that prevents *all* argument
conversions, as investigated in the conda-forge `cairo` package:
conda-forge/cairo-feedstock#18.

I do not know what fallout this change will have for existing build scripts,
but the current behavior cannot be what was intended.
@mingwandroid
Copy link
Contributor

This is strange, I put fixes into MSYS2 specifically for what you describe. Unfortunately I don't have much time to look into it, so I'd say merge it anyway.

@pkgw
Copy link
Contributor Author

pkgw commented Jan 11, 2017

Perhaps that's related to another thing I noticed in my testing? The cygpath.exe provided in the Miniconda3 installer that we use as a basis in our CI builders turns Windows paths into things like /cygdrive/c/.... But the cygpath.exe that gets installed when actually building the packages doesn't use the prefix: /c/..... This leads me to suspect that there are two sources of MSYS2 that have gotten out of sync in some way — I don't know how the conda-forge project is getting its MSYS2 packages.

@mingwandroid
Copy link
Contributor

Git for Windows is based on MSYS2 and is (obviously) installed on CI machines.

So if you are using Git as a build dependency in your package you need to use m2-git if you also use any other msys2 packages (like posix).

Other than that, if you haven't installed the m2-filesystem package (it is a dependency of posix which is the easiest way to get msys2 tools) then you'll see /cygdrive instead of /c.

@msarahan
Copy link
Contributor

cygpath.exe is not provided in miniconda, I think. @pkgw can you please verify that? I think it's just part of the standard AppVeyor installation, which is where you're picking it up from.

@mingwandroid
Copy link
Contributor

It is part of m2-msys2-runtime which is a dependency of posix:
https://github.com/pkgw/cairo-feedstock/blob/82f153ed5e60259518936e81838466ef0e5ab56c/recipe/meta.yaml#L22-L23

@pkgw
Copy link
Contributor Author

pkgw commented Jan 11, 2017

Yes, right, to be more precise: the cygpath.exe provided by the m2-msys2-runtime package that gets installed if I set up a Miniconda3 environment and install that package.

But here's what I'm talking about. On my Windows VM, I set up a demo "package" with a dependency on posix and a bld.bat that looks like this:

@echo on
%CONDA_ROOT%\Library\usr\bin\cygpath.exe C:\Miniconda3
%PREFIX%\Library\usr\bin\cygpath.exe C:\Miniconda3
exit 1

The first invocation uses the cygpath with my VM's Miniconda + m2-msys2-runtime + conda-forge install; the second uses the version that's invoked as part of the build process. The first one produces a path with the /cygdrive prefix; the second doesn't.

(I guess it bears emphasis that all of this may or may not have anything to do with this pull request, but if there's supposed to be a patch that fixes this problem in MSYS2, I do wonder if there's some kind of mixed package versioning going on.)

@mingwandroid
Copy link
Contributor

Your VM's Miniconda install probably does not have the m2-filesystem installed. Can you install that and try again?

@pkgw
Copy link
Contributor Author

pkgw commented Jan 11, 2017

Ah, yes, that makes things consistent. Should m2-filesystem be a dep of m2-msys2-runtime?

Well, that clears up that particular thing, but still leaves the possible issue with trailing semicolons. If the AppVeyor build of conda-forge Cairo ever runs (yesterday it sat on the queue for like 14 hours) I'll be able to check that it's still a problem, in that particular build setup, at least ...

@mingwandroid
Copy link
Contributor

No IMHO it shouldn't, m2-msys2-runtime is the closet equivalent to the Linux kernel (it's a light fork of the cygwin DLL), plus the bare minimum of tools to support it.

@msarahan
Copy link
Contributor

@pkgw can you confirm whether Cairo ran successfully? Is this PR still necessary?

@pkgw
Copy link
Contributor Author

pkgw commented Jan 18, 2017

Yes, with that Cairo PR, the equivalent of this change was the difference between success and failure. So, to the best of my knowledge, this PR is necessary, unless something about conda-build has changed in the past few days.

In principle it'd be interesting to understand if/why the fix alluded to by @mingwandroid didn't work or isn't active on the conda-forge builders, though.

@msarahan
Copy link
Contributor

All right, I'm going to merge this. I kind of doubt we've seen the last of this issue, though. If we revisit it, I think we might do better to have Python check for that variable in os.environ, and if set, do the concatenation itself and place the value directly as a string with no env var component.

@msarahan msarahan merged commit f46cbea into conda:master Jan 20, 2017
@pkgw pkgw deleted the pr-msys2-arg-conv-excl branch January 20, 2017 04:03
@github-actions
Copy link

github-actions bot commented May 6, 2022

Hi there, thank you for your contribution!

This pull request has been automatically locked because it has not had recent activity after being closed.

Please open a new issue or pull request if needed.

Thanks!

@github-actions github-actions bot added the locked [bot] locked due to inactivity label May 6, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants