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

support setting environment variables #198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mmomtchev
Copy link

This adds support for setting environment variables when running actions

Unlike the commands, which are expanded in a single LiquidJS instance, the variables are expanded in separate instances. This is probably worth discussing.

@ilg-ul
Copy link
Contributor

ilg-ul commented May 23, 2024

Does npm support something similar?

@mmomtchev
Copy link
Author

None that I can think of.

@ilg-ul
Copy link
Contributor

ilg-ul commented May 23, 2024

I can't check now, but I remember when I checked some years ago, there were some references to passing configuration via shell environment variables.

I think that generally we should remain consistent.

Copy link
Author

@mmomtchev mmomtchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass npm options, for example, npm --build-from-source - these will be made available to the action script in its environments as npm_config_build_from_source=true. Maybe it was this?

@mmomtchev
Copy link
Author

The problem does not have a generic solution:
https://stackoverflow.com/questions/37140799/passing-environment-variables-in-npm-scripts

There are a number of npm packages that do this for plain npm actions.

@ilg-ul
Copy link
Contributor

ilg-ul commented May 23, 2024

Unlike the commands, which are expanded in a single LiquidJS instance, the variables are expanded in separate instances. This is probably worth discussing.

Can you elaborate on the rationale behind this proposal? Do you have a use case that you can share?

@mmomtchev
Copy link
Author

I am trying to reduce the complexity of https://github.com/mmomtchev/magickwand.js/blob/meson/package.json

Clean cross-platform method for setting the environment variables will help a lot.

I am installing custom versions of conan and meson and setting PYTHONHOME and CONAN_HOME. Your method of installing meson as a package would solve the problem for meson but there is no solution for conan.

@ilg-ul
Copy link
Contributor

ilg-ul commented May 23, 2024

I am trying to reduce the complexity of ...

I see. Pretty hard to read. :-(

Can you provide a similar package.json using your solution, to see the improvement?

Clean cross-platform method for setting the environment variables will help a lot.

Aha. So the goal is to define a method for setting environment variables to be consumed by tools invoked in actions.

I have to think about it.

@ilg-ul
Copy link
Contributor

ilg-ul commented May 23, 2024

BTW, as far as I understood it, the philosophy of npm is to unify all platforms by bringing links to all dependencies in the .bin folder, such that the same tool can be invoked using the same command on all platforms.

This generally works fine, except on Windows, where .cmd shims require the use of a cmd.exe sub-shell, and not all tools (for example cmake) do this.

Seeing your use case, with lots of platform specific separate definitions, made me wonder what can be done for it to benefit from a similar unifying approach? Would a mechanism for setting environment variables be enough?

@mmomtchev
Copy link
Author

  • .cmd is another problem. This comes from Node.js itself, which uses a spawn mechanism that requires the .cmd. I think that launching processes using a shell should resolve it?
  • Python is, alas, unavoidable. They have made the decision to use python on Windows and python3 on macOS. Linux generally accepts both.
  • Setting environment variables is very ugly since on Windows it is a separate command while on macOS / Linux it is a command prefix
  • There are also some other commands such as the importing of the conan environment variables

Another solution that I was thinking of was a xPack with bash - this will also solve all cross-platform command-line problems.

@ilg-ul
Copy link
Contributor

ilg-ul commented May 23, 2024

.cmd is another problem. This comes from Node.js itself, which uses a spawn mechanism that requires the .cmd. I think that launching processes using a shell should resolve it?

This solution is both a blessing and a curse. It is a blessing since it made npm on Windows possible, compensating for the lack of links to files. It is a curse since it requires the shell. For npm/xpm this is not a problem, but otherwise it is a nuisance, like having to explicitly suffix the command name with .cmd, while on other platforms the name alone is enough (I use this in all my cmake scripts to invoke the compilers, for example).

Setting environment variables is very ugly since on Windows it is a separate command while on macOS / Linux it is a command prefix

Right. And this is one aspect where your proposal makes things a bit cleaner.

... a xPack with bash - this will also solve all cross-platform command-line problems.

Yeah, possibly. There is a sh in the Windows Build Tools package, you can experiment with it, and if you find an elegant solution, we can think of a new xPack with bash. Did you experiment with a local bash (installed via msys2, for example) and it was a realistic solution?

BTW, in the npm world there is also a portable shell (shx); I use it occasionally, it seems fine.

@mmomtchev
Copy link
Author

Alas, shx does not support setting environment variables - in fact, it has a very limited subset of the bash features.

However sh from the Windows Build Tools seems to do the job and allows using the same commands on all three major OS.

@ilg-ul
Copy link
Contributor

ilg-ul commented May 24, 2024

However sh from the Windows Build Tools seems to do the job and allows using the same commands on all three major OS.

Nice! Can you share the resulting package.json that uses sh on all platforms? And possibly the one that also uses environment variables, so I have an idea where we are aiming?

If you think it is an improvement, it probably makes sense to have a bash package. It remains to be seen how difficult building it for Windows will be.

@mmomtchev
Copy link
Author

It is only now that I understood the meson package (I discovered the meson.c file), I will try to do the same with conan.

@ilg-ul
Copy link
Contributor

ilg-ul commented May 24, 2024

I will try to do the same with conan.

Well, I'm reticent to encourage you to do so. For my use case (embedded projects with extensive testing), I currently do not need any Python at all, thus a standalone meson makes some sense.

For your use case probably having a plain Python around is a more realistic solution.

Actually I do not understand the details of your build, thus a more conservative approach.

As a side note, if for my projects I'll ever need Python, I'm considering a Python3 xPack, to be installed the same on all platforms.

@ilg-ul ilg-ul deleted the branch xpack:master November 21, 2024 13:10
@ilg-ul ilg-ul closed this Nov 21, 2024
@mmomtchev
Copy link
Author

@ilg-ul, I have another use case for this: my static Python xPack now comes with root CA certificates - this was a problem mainly on macOS where it required that OpenSSL 1 was installed from homebrew. Now I don't need that the root certificates are installed on macOS and Linux - Windows is not a problem since it has its standard certificate store. The problem is that I need to set the SSL_CERT_FILE environment variable. One way to do it is to use a wrapper - since I need this only on Linux and macOS, this is not that difficult: https://github.com/mmomtchev/static-portable-python/pull/11/files

However this has drawbacks and I wonder if there isn't a better way to do it. Patching Python is also an option, but this is not a trivial patch since Python will need to find the location of its own executable from within the C code of the ssl module.

@ilg-ul
Copy link
Contributor

ilg-ul commented Jan 20, 2025

I plan to update xpm, hopefully in 2-3 weeks, and include this feature too. I just need to complete development on the documentation sites.

@ilg-ul ilg-ul reopened this Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants