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

Environment variables in $GITHUB_ENV ignored for PATH and PKG_CONFIG_PATH #98

Closed
eyal0 opened this issue Dec 25, 2020 · 20 comments
Closed
Labels
enhancement New feature or request question Further information is requested

Comments

@eyal0
Copy link

eyal0 commented Dec 25, 2020

I'm using $GITHUB_ENV to modify environment variables. Sometimes it works, sometimes it doesn't.

In my CI yaml file, I have this:

    - name: Setup paths and env
      run: |
        mkdir -p $HOME/.local/bin
        mkdir -p $HOME/.local/lib/pkgconfig
        echo "PKG_CONFIG_PATH=$HOME/.local/lib/pkgconfig:$PKG_CONFIG_PATH" >> $GITHUB_ENV
        echo "LD_LIBRARY_PATH=$HOME/.local/lib:$LD_LIBRARY_PATH" >> $GITHUB_ENV
        echo "PATH=$HOME/.local/bin:$PATH" >> $GITHUB_ENV
        echo "ANT_HOME=foo" >> $GITHUB_ENV
        echo "ANT_HOME2=bar" >> $GITHUB_ENV

And later on in the same ci, I have this:

    - name: Display information about build environment
      continue-on-error: true
      run: |
        env
        g++ --version
        clang++ --version
        pkg-config --version
        m4 --version

The first one should put some new variables into $GITHUB_ENV. Those variable then become part of the execution environment for the second one. We can see that it somewhat works. Here's the log of the CI run:

2020-12-25T16:24:59.5787589Z ##[group]Run env
2020-12-25T16:24:59.5788080Z �[36;1menv�[0m
2020-12-25T16:24:59.5788444Z �[36;1mg++ --version�[0m
2020-12-25T16:24:59.5788831Z �[36;1mclang++ --version�[0m
2020-12-25T16:24:59.5789451Z �[36;1mpkg-config --version�[0m
2020-12-25T16:24:59.5789887Z �[36;1mm4 --version�[0m
2020-12-25T16:24:59.5792315Z shell: D:\a\_temp\msys\msys2.CMD {0}
2020-12-25T16:24:59.5792732Z env:
2020-12-25T16:24:59.5793218Z   MSYSTEM: MINGW64
2020-12-25T16:24:59.5793969Z   PKG_CONFIG_PATH: /home/runneradmin/.local/lib/pkgconfig:/mingw64/lib/pkgconfig:/mingw64/share/pkgconfig
2020-12-25T16:24:59.5794828Z   LD_LIBRARY_PATH: /home/runneradmin/.local/lib:
2020-12-25T16:24:59.5796094Z   PATH: /home/runneradmin/.local/bin:/mingw64/bin:/usr/local/bin:/usr/bin:/bin:/c/Windows/System32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0/:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl
2020-12-25T16:24:59.5797320Z   ANT_HOME: foo
2020-12-25T16:24:59.5797714Z   ANT_HOME2: bar
2020-12-25T16:24:59.5798095Z   NUM_CPUS: 8
2020-12-25T16:24:59.5798458Z ##[endgroup]
2020-12-25T16:24:59.9898476Z USERDOMAIN=fv-az177-439
2020-12-25T16:24:59.9899859Z OS=Windows_NT
2020-12-25T16:24:59.9901764Z LD_LIBRARY_PATH=/home/runneradmin/.local/lib:
2020-12-25T16:24:59.9904727Z COMMONPROGRAMFILES=C:\Program Files\Common Files
2020-12-25T16:24:59.9919605Z PROCESSOR_LEVEL=6
2020-12-25T16:24:59.9921928Z PSModulePath=C:\Modules\azurerm_2.1.0;C:\Modules\azure_2.1.0;C:\Users\packer\Documents\WindowsPowerShell\Modules;C:\Program Files\WindowsPowerShell\Modules;C:\windows\system32\WindowsPowerShell\v1.0\Modules;C:\Program Files\Microsoft SQL Server\130\Tools\PowerShell\Modules\;C:\Program Files (x86)\Google\Cloud SDK\google-cloud-sdk\platform\PowerShell
2020-12-25T16:25:00.0211451Z ANT_HOME2=bar
2020-12-25T16:25:00.0265443Z ANT_HOME=foo
2020-12-25T16:25:00.0272362Z PKG_CONFIG_PATH=/mingw64/lib/pkgconfig:/mingw64/share/pkgconfig
2020-12-25T16:25:00.0267298Z PATH=/mingw64/bin:/usr/local/bin:/usr/bin:/bin:/c/Windows/System32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0/:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl

(I didn't include all the lines in the above log to keep it brief. Full log here.)

Notice how the environment for this command includes setting variables like MSYSTEM, PATH, LD_LIBRARY_PATH and others in the very first lines. And then when we view the results of env, some of those were set into the environment correctly, such as ANT_HOME and ANT_HOME2 and LD_LIBRARY_PATH. But others, like PKG_CONFIG_PATH and PATH were not taken from the environment.

I expected that all the values in the environment would make it into the result of env, not just some of them.

@lazka
Copy link
Member

lazka commented Dec 25, 2020

Letting PATH leak into the environment would break things, so that's off by default. PKG_CONFIG_PATH needs to be extended in msys2, not sure if we should prepend it instead of replacing it..

For PATH you can set path-type: inherit, but you are on your own and have to make sure there are no DLL conflicts.

@eyal0
Copy link
Author

eyal0 commented Dec 25, 2020

When you say PATH is leaking, do you mean that the PATH environment variable outside of msys would leak into msys?

What solution would you suggest to me? I would like to append to PATH and PKG_CONFIG_PATH for the whole CI and writing into GITHUB_ENV works for macos and ubuntu CI already. I would like a solution that is consistent.

Maybe with modifying the default.run.shell?

@eine
Copy link
Collaborator

eine commented Dec 25, 2020

My suggestion is to write your settings to a file and then source it. That allows you to define the envvars you want only, and not all the PATH/env. You could use bashrc.

@eyal0
Copy link
Author

eyal0 commented Dec 25, 2020

That's the idea that I had, too. I would like it to be somewhat automatic... Here is what I am trying:

    defaults:
      run:
        shell: msys2 -c 'source $GITHUB_ENV; export $(cut -d= -f1 $GITHUB_ENV); {0}'

I'll report here how it goes.

@eine
Copy link
Collaborator

eine commented Dec 25, 2020

Please, let us know how it goes. Since this Action is very specifically to be used on GitHub Actions, I think it might make sense to add that feature into https://github.com/msys2/setup-msys2/blob/master/main.js#L164-L168 (making it optional).

@eyal0
Copy link
Author

eyal0 commented Dec 25, 2020

I'll let you know. I'm not sure how that shell line is processed. I think that it may depend whether the github action to be invoked is a single line or multiple lines. I read that for just one line, the command is substituted in, but for a multiline command, a file is created and that file is run.

I'll keep looking at it. It would be nice to have a shell command that supports the $GITHUB_ENV and also works for all three of windows, ubuntu, and macos. Like maybe it could check if msys2 exists first and use it only if needed? I'm not sure if it's possible to do that from github actions yaml though maybe it could be done in a way that works on both bash and powershell...

@eine
Copy link
Collaborator

eine commented Dec 25, 2020

On the one hand, GitHub maintaners are looking into making MSYS2 the default bash on Windows runners (instead of Git for Windows' bash). However, they added MSYS2 to the PATH, which caused issues, and they had to revert back. So, I would expect some changes in the following months, but I believe that setup-msys2 will still be useful for allowing users to control the setup.

On the other hand, it should be possible to write an Action that generates a shell entrypoint (similar to msys2 in here) taking the environment into account. That is shell: myshell {0}, and then have it behave the same regardless of the OS. Yet, I think that's out of scope of this repo.

@eyal0
Copy link
Author

eyal0 commented Dec 25, 2020

I still have some ideas. If I find something workable, I will let you know for sure!

@eine eine added enhancement New feature or request question Further information is requested labels Dec 25, 2020
@eyal0
Copy link
Author

eyal0 commented Dec 25, 2020

My idea above didn't work because {0} is a filename. So I need source {0}

But that doesn't work either because it's a windows path so I need to call cygpath on it. But I can't call the cygpath from inside msys because by then, all the backslashes are already removed.

I will keep trying.

@eyal0
Copy link
Author

eyal0 commented Dec 26, 2020

I'm making some progress.

I'm trying to figure out how msys is handling the script that github is creating.

Github takes the run parameter in the CI and puts all of it into a script. In the case of windows, it puts this on the first line:

$ErrorActionPreference = 'stop'

and on the last line this:

if ((Test-Path -LiteralPath variable:\LASTEXITCODE)) { exit $LASTEXITCODE }

The file that starts and ends with those lines and in the middle has a bunch of unix commands. Then that file is run with msys2 like this: msys2 the_file.ps1

How is it that msys can handle those start and end lines? I expected that it would fail because those aren't valid code for bash.

@eine
Copy link
Collaborator

eine commented Dec 26, 2020

@eyal0, I think that you are checking the behaviour with a powershell run (the default if you don't specify shell:). Can you test it with shell: cmd and/or shell: bash? Also, I'd be interested in knowing how did you get to "see" those lines (or the reference where you read about it).

My understanding is that using custom shells (such as msys2's) the set of unix commands is copied into a file without any additional content.

@eyal0
Copy link
Author

eyal0 commented Dec 27, 2020

@eine I saw it while using a custom shell. My understanding is that when you select a shell, it can either be custom or not. A custom shell is one where the shell includes {0} in it. For a custom shell, that {0} will be replaced by the name of a file which contains all the code that needs to run.

(You can also use non-custom shells like bash or cmd or powershell, etc. Those aliases for stuff like /bin/bash --norc {0}. If you use a non-custom shell, it must be one of those few keywords.)

I installed a custom shell that does a bunch of stuff to workaround the PATH and PKG_CONFIG_PATH difficulties. In my testing, I am able to print out the script file. You can see it here:

https://github.com/eyal0/universal-ci-shell/runs/1612506932?check_suite_focus=true

The custom shell is:

          powershell -command "[System.Environment]::SetEnvironmentVariable('GITHUB_SCRIPT', ('{0}' -replace '\\','\\')); dir Env:; echo WHAT IS THE SCRIPT CONTENTS; type {0}; echo THE END"
          msys2 -c 'echo ORIGINAL SCRIPT BELOW; cat $(cygpath $GITHUB_SCRIPT); echo END OF SCRIPT; if [[ -v MSYSTEM ]]; then sed -i 1d $(cygpath $GITHUB_SCRIPT); sed -i \$d $(cygpath $GITHUB_SCRIPT); source $(cygpath $GITHUB_SCRIPT); fi'

The shell first copies the filename of the script (which is a MSDOS path) into an environment variable. That variable is copied into the msys2 environment and cygpath is used to convert it to a Unix path. The shell also prints out the script both from powershell and from msys2. You can see it in the log or in the pictures below.

image

See how the file starts with line and ends with that other one? I think that Github put it in there. I'm not sure if the behavior is the same in ubuntu or macos CI. Also I don't understand how msys2 isn't complaining about those lines.

@eyal0
Copy link
Author

eyal0 commented Dec 27, 2020

@eine Okay so it's a hack and not pretty but I have a solution that is letting me run the same CI steps in macos, ubuntu, and windows (msys2). The steps do some manipulation of the environment variables and make some symlinks. I have it set up as a CI in github that first configures the CI and then runs a list of commands and checks the results. For all three platforms, all the checks pass using identical steps.

What I do is first invoke powershell to copy the location of the github script to an environment variable. I also make copies of PKG_CONFIG_PATH and PATH because msys2 will not copy those from the powershell environment into the msys2 environment but it will import the copies.

Then we run msys2 with a long command that copies the environment variables back into place, but only if they exist. Also, some sed needs to be done on the environment variables to format them correctly for msys2.

Now the tests pass for Windows. The problem was that they have to pass for linux and macos, too! Well, the new default shell command calls powershell once and msys2 once. The call to powershell is not needed for macos nor Ubuntu so a symlink from powershell to /bin/true is created. That causes the call to powershell to do nothing. And msys2 is symlinked to bash. The argument to msys2 was written carefully to use no single-quotes and no double-quotes. That is because msys2 runs from windows command line and bash would run from bash and they have different ways of quoting and escaping quotes. So to avoid this, there are no quotes in the whole thing.

The test checks that environment variables can be set, replaced, modified, etc. Also, the PATH can be modified in two ways: either by writing to $GITHUB_ENV or by writing to $GITHUB_PATH. The test does a combination of those methods and confirms that the result is as expected. And the results are the same for all three platforms.

If I discover more things that need to be done so that GitHub CI can be compatible between all three platforms, I'll add them.

Would this be useful for people?

The alternative to this method is to just writing one long script that does all the CI and instead of making a bunch of CI steps, just call a single script. That will be more compatible. The disadvantage is that you lose all the metadata that the GitHub CI system has, such as separating the steps and skipping steps depending on the configuration.

An advantage to my solution is that you can just add a few lines to an existing CI and hopefully the CI will start to work for windows, macos, and Ubuntu, all three of them.

If GitHub made a few small changes to their CI, we could do away with all this stuff! But they would need to do that.

@eyal0
Copy link
Author

eyal0 commented Jan 10, 2021

Okay, I've got a solution that work for me now, combining my ideas above plus idea from #102 . Here's the key to it:

    strategy:
      matrix:
        os: [ubuntu, macos, windows]
        include:
          - os: windows
            shell: |
              powershell -command "[System.Environment]::SetEnvironmentVariable('GITHUB_SCRIPT', ('{0}' -replace '\\','\\')); [System.Environment]::SetEnvironmentVariable('PKG_CONFIG_PATH_MSYS', $Env:PKG_CONFIG_PATH); [System.Environment]::SetEnvironmentVariable('PATH_MSYS', $Env:Path);"
              msys2 -c 'if [[ -v MSYSTEM ]]; then sed -i 1d $(cygpath $GITHUB_SCRIPT); sed -i \$d $(cygpath $GITHUB_SCRIPT); if [[ -v PKG_CONFIG_PATH_MSYS ]]; then export PKG_CONFIG_PATH=$PKG_CONFIG_PATH_MSYS; fi; if [[ -v PATH_MSYS && ! ( $PATH_MSYS =~ ^D:\\\\a\\\\_temp\\\\msys\\;C:\\\\Users ) ]]; then export PATH=$(echo $PATH_MSYS | sed s/D:\\\\\\\\a\\\\\\\\_temp\\\\\\\\msys\;// | sed s/\;/:/g); fi; fi; source $(cygpath $GITHUB_SCRIPT)'
          - os: ubuntu
            shell: bash
          - os: macos
            shell: bash
    runs-on: ${{ matrix.os }}-latest
    defaults:
      run:
        shell: ${{ matrix.shell }}

With all that, you can use pretty much the same steps for both Windows/msys2 as for macos and linux. Specifically, GITHUB_PATH and GITHUB_ENV will be honored, as will any changed to PKG_CONFIG_PATH. You can see it in action here: https://github.com/pcb2gcode/pcb2gcode/actions

Based on this, I should also be fixing MANPATH and AC_LOCAL_PATH but that didn't come up in my work. It could be fixed in the same way as PKG_CONFIG_PATH as above.

You maybe still need some if conditions to fix up places where Windows and Unix don't behave the same. For example, brew versus apt-get versus pacman, but that's to be expected.

Enjoy!

@eyal0 eyal0 closed this as completed Jan 10, 2021
@eine
Copy link
Collaborator

eine commented Jan 10, 2021

@eyal0 you might want to extend this Action with those two lines, in order to make them easier to maintain and reuse. I would suggest the following:

  1. Fork this repo.
  2. Create a feature branch.
  3. Locally, execute npm run install. Either get NodeJs, use a node container, or use WSL.
  4. Edit https://github.com/msys2/setup-msys2/blob/master/main.js#L163-L174, so that an additional file is written, which contains your custom code.
  5. Execute npm run pkg.
  6. Add index.js, commit and push.
  7. Edit your workflow for using eyal0/setup-msys2@feature-branch-name and change the shell: yourcustomentrypoint {0}. Commit and push.
  8. Go back to 4-6, and restart the worflow until you get it done.

@eyal0
Copy link
Author

eyal0 commented Jan 11, 2021

@eyal0 you might want to extend this Action with those two lines, in order to make them easier to maintain and reuse. I would suggest the following:

If I did that, would you merge it into this repo? Or should I just maintain my own fork?

@eine
Copy link
Collaborator

eine commented Jan 11, 2021

What I explained would be the first step towards having it merged into this repo. Having yourcustomentrypoint written in a file, instead of in a single-row (actually, two), would make it easier to review and to suggest enhancements. For instance, I wonder whether sed commands can be replaced with either cygpath -w or cygpath -u. Unfortunately, it's difficult to understand what's going on at the moment, and shell: | places the commands at an unknown location.

@eine
Copy link
Collaborator

eine commented Jan 11, 2021

Note that you don't need to fork setup-msys2 in order to have yourcustomentrypoint available at a known location as a *.cmd file. But I believe it's easier to fork than guessing the custom step for setting that up.

@eine
Copy link
Collaborator

eine commented Jan 11, 2021

See also actions/runner-images#2395. After that is deployed, we might want to rethink how we deal with msys2.cmd. We are not forced to do so, but it'd be sensible.

@eyal0
Copy link
Author

eyal0 commented Jan 11, 2021

@eine I never figured out what's going on here: #98 (comment)

How come the scripts have that line added at the top and bottom. And how come msys2 is unaffected? It's strange to me.

Do you know where I could search inside of the GitHub actions to find if those lines are being added somewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants