-
Notifications
You must be signed in to change notification settings - Fork 981
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
Add Fish support for environments #15503
base: develop2
Are you sure you want to change the base?
Changes from 8 commits
5f19714
bf5d21e
b249c6d
e16bf22
d42b388
269b4c8
e703b07
88a48b4
c60eb5a
1ab3a8d
70c34ae
37eab05
e32b420
8cdfd29
966c631
dd517c6
325fe06
2d202ed
66ca6a2
f57f32f
4535b74
32125de
cd94ce3
35d682e
d55eff5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,7 +1,10 @@ | ||||
import os | ||||
import textwrap | ||||
from jinja2 import Template | ||||
from collections import OrderedDict | ||||
from contextlib import contextmanager | ||||
from pathlib import Path | ||||
|
||||
|
||||
from conans.client.generators import relativize_paths | ||||
from conans.client.subsystems import deduce_subsystem, WINDOWS, subsystem_path | ||||
|
@@ -19,16 +22,20 @@ def environment_wrap_command(env_filenames, env_folder, cmd, subsystem=None, | |||
if not env_filenames: | ||||
return cmd | ||||
filenames = [env_filenames] if not isinstance(env_filenames, list) else env_filenames | ||||
bats, shs, ps1s = [], [], [] | ||||
bats, shs, ps1s, fishs = [], [], [], [] | ||||
|
||||
accept = accepted_extensions or ("ps1", "bat", "sh") | ||||
accept = accepted_extensions or ("ps1", "bat", "sh", "fish") | ||||
# TODO: This implemantation is dirty, improve it | ||||
for f in filenames: | ||||
f = f if os.path.isabs(f) else os.path.join(env_folder, f) | ||||
if f.lower().endswith(".sh"): | ||||
if os.path.isfile(f) and "sh" in accept: | ||||
f = subsystem_path(subsystem, f) | ||||
shs.append(f) | ||||
elif f.lower().endswith(".fish"): | ||||
if os.path.isfile(f) and "fish" in accept: | ||||
f = subsystem_path(subsystem, f) | ||||
fishs.append(f) | ||||
elif f.lower().endswith(".bat"): | ||||
if os.path.isfile(f) and "bat" in accept: | ||||
bats.append(f) | ||||
|
@@ -39,17 +46,21 @@ def environment_wrap_command(env_filenames, env_folder, cmd, subsystem=None, | |||
path_bat = "{}.bat".format(f) | ||||
path_sh = "{}.sh".format(f) | ||||
path_ps1 = "{}.ps1".format(f) | ||||
path_fish = "{}.fish".format(f) | ||||
if os.path.isfile(path_bat) and "bat" in accept: | ||||
bats.append(path_bat) | ||||
if os.path.isfile(path_ps1) and "ps1" in accept: | ||||
ps1s.append(path_ps1) | ||||
if os.path.isfile(path_sh) and "sh" in accept: | ||||
path_sh = subsystem_path(subsystem, path_sh) | ||||
shs.append(path_sh) | ||||
if os.path.isfile(path_fish) and "fish" in accept: | ||||
path_fish = subsystem_path(subsystem, path_fish) | ||||
fishs.append(path_fish) | ||||
|
||||
if bool(bats + ps1s) + bool(shs) > 1: | ||||
if bool(bats + ps1s) + bool(shs) > 1 + bool(fishs) > 1: | ||||
raise ConanException("Cannot wrap command with different envs," | ||||
"{} - {}".format(bats+ps1s, shs)) | ||||
"{} - {} - {}".format(bats+ps1s, shs, fishs)) | ||||
|
||||
if bats: | ||||
launchers = " && ".join('"{}"'.format(b) for b in bats) | ||||
|
@@ -62,6 +73,10 @@ def environment_wrap_command(env_filenames, env_folder, cmd, subsystem=None, | |||
elif shs: | ||||
launchers = " && ".join('. "{}"'.format(f) for f in shs) | ||||
return '{} && {}'.format(launchers, cmd) | ||||
elif fishs: | ||||
launchers = " && ".join('. "{}"'.format(f) for f in fishs) | ||||
print(f"LAUNCHERS: {launchers}") | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
return 'fish -c "{} && {}"'.format(launchers, cmd) | ||||
elif ps1s: | ||||
# TODO: at the moment it only works with path without spaces | ||||
launchers = " ; ".join('"&\'{}\'"'.format(f) for f in ps1s) | ||||
|
@@ -520,12 +535,64 @@ def save_sh(self, file_location, generate_deactivate=True): | |||
content = f'script_folder="{os.path.abspath(filepath)}"\n' + content | ||||
save(file_location, content) | ||||
|
||||
def save_fish(self, file_location): | ||||
"""Save a fish script file with the environment variables defined in the Environment object. | ||||
|
||||
It generates a function to deactivate the environment variables configured in the Environment object. | ||||
|
||||
:param file_location: The path to the file to save the fish script. | ||||
""" | ||||
filepath, filename = os.path.split(file_location) | ||||
function_name = f"deactivate_{Path(filename).stem}" | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the function_name is this one? Why not just the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It follow the common name used by other virtualenv like:
In Fish, functions are exported as a command, so you can run directly. About the random to avoid collision: In case we have multiple dependencies add bin folder to PATH, we will need to undo the configuration after deactivating it. Usually it would be store in a deactivate script, but here we are using a variable. Actually, the random is just an idea, but could be the build folder name too, because is kind unique name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, having something deterministic would make more sense, I probably still didn't understand the issue. |
||||
script_content = Template(textwrap.dedent("""\ | ||||
function {{ function_name }} | ||||
echo "echo Restoring environment" | ||||
{% for item in vars_unset %} | ||||
set -e {{ item }} | ||||
{% endfor %} | ||||
{% for item, value in vars_restore.items() %} | ||||
set -gx {{ item }} {{ value }} | ||||
{% endfor %} | ||||
end | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The restore should work restoring the variables that were defined at activation time, not the variables that were defined at the time of generating the script. It might be a bit more complicated, the activation needs to store the variables in memory or something too, so the restore can use them |
||||
{% for item, value in vars_prepend.items() %} | ||||
set -pgx {{ item }} '{{ value }}' | ||||
{% endfor %} | ||||
{% for item, value in vars_define.items() %} | ||||
set -gx {{ item }} '{{ value }}' | ||||
{% endfor %} | ||||
""")) | ||||
values = self._values.keys() | ||||
vars_unset = [] | ||||
vars_restore = {} | ||||
vars_prepend = {} | ||||
vars_define = {} | ||||
for varname, varvalues in self._values.items(): | ||||
current_value = os.getenv(varname) | ||||
abs_base_path, new_path = relativize_paths(self._conanfile, "$script_folder") | ||||
value = varvalues.get_str("", self._subsystem, pathsep=self._pathsep, | ||||
root_path=abs_base_path, script_path=new_path) | ||||
value = value.replace('"', '\\"') | ||||
if current_value: | ||||
vars_restore[varname] = current_value | ||||
vars_prepend[varname] = value | ||||
else: | ||||
vars_define[varname] = value | ||||
vars_unset.append(varname) | ||||
|
||||
if values: | ||||
content = script_content.render(function_name=function_name, vars_unset=vars_unset, | ||||
vars_restore=vars_restore, vars_prepend=vars_prepend, | ||||
vars_define=vars_define) | ||||
save(file_location, content) | ||||
|
||||
def save_script(self, filename): | ||||
""" | ||||
Saves a script file (bat, sh, ps1) with a launcher to set the environment. | ||||
If the conf "tools.env.virtualenv:powershell" is set to True it will generate powershell | ||||
launchers if Windows. | ||||
|
||||
If the conf "tools.env.virtualenv:fish" is set to True it will generate fish launchers. | ||||
|
||||
:param filename: Name of the file to generate. If the extension is provided, it will generate | ||||
the launcher script for that extension, otherwise the format will be deduced | ||||
checking if we are running inside Windows (checking also the subsystem) or not. | ||||
|
@@ -534,18 +601,27 @@ def save_script(self, filename): | |||
if ext: | ||||
is_bat = ext == ".bat" | ||||
is_ps1 = ext == ".ps1" | ||||
is_fish = ext == ".fish" | ||||
else: # Need to deduce it automatically | ||||
is_bat = self._subsystem == WINDOWS | ||||
is_ps1 = self._conanfile.conf.get("tools.env.virtualenv:powershell", check_type=bool) | ||||
if is_ps1: | ||||
is_fish = self._conanfile.conf.get("tools.env.virtualenv:fish", check_type=bool) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are no wrappers for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, please, take a look in the test |
||||
if is_fish: | ||||
filename = filename + ".fish" | ||||
is_bat = False | ||||
is_ps1 = False | ||||
elif is_ps1: | ||||
filename = filename + ".ps1" | ||||
is_bat = False | ||||
is_fish = False | ||||
else: | ||||
filename = filename + (".bat" if is_bat else ".sh") | ||||
|
||||
path = os.path.join(self._conanfile.generators_folder, filename) | ||||
if is_bat: | ||||
self.save_bat(path) | ||||
elif is_fish: | ||||
self.save_fish(path) | ||||
elif is_ps1: | ||||
self.save_ps1(path) | ||||
else: | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -159,6 +159,7 @@ def deactivates(filenames): | |
bats = [] | ||
shs = [] | ||
ps1s = [] | ||
fishs = [] | ||
for env_script in env_scripts: | ||
path = os.path.join(conanfile.generators_folder, env_script) | ||
# Only the .bat and .ps1 are made relative to current script | ||
|
@@ -167,6 +168,8 @@ def deactivates(filenames): | |
bats.append("%~dp0/"+path) | ||
elif env_script.endswith(".sh"): | ||
shs.append(subsystem_path(subsystem, path)) | ||
elif env_script.endswith(".fish"): | ||
fishs.append(subsystem_path(subsystem, path)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, no subsystem for fish initially, IMHO |
||
elif env_script.endswith(".ps1"): | ||
path = os.path.relpath(path, conanfile.generators_folder) | ||
# This $PSScriptRoot uses the current script directory | ||
|
@@ -179,6 +182,15 @@ def sh_content(files): | |
save(os.path.join(conanfile.generators_folder, filename), sh_content(shs)) | ||
save(os.path.join(conanfile.generators_folder, "deactivate_{}".format(filename)), | ||
sh_content(deactivates(shs))) | ||
if fishs: | ||
def fish_content(files): | ||
return ". " + " && . ".join('"{}"'.format(s) for s in files) | ||
fishs = [it for it in fishs if os.path.isfile(it)] | ||
if fishs: | ||
filename = "conan{}.fish".format(group) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here it seems it is missing the deactivation function (not script)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually the deactivation is not an script, but a function exported by Fish in the environment. So we can call the deactivate_conanbuild, but there is no file associated. The function is created on-the-fly: https://github.com/conan-io/conan/pull/15503/files#diff-56b36c23589a66b0bd803c545703684c74400cbb7bd8538b8949ed84613e3fc7R550 |
||
generated.append(filename) | ||
save(os.path.join(conanfile.generators_folder, filename), fish_content(fishs)) | ||
# TODO : Deactivate fish | ||
if bats: | ||
def bat_content(files): | ||
return "\r\n".join(["@echo off"] + ['call "{}"'.format(b) for b in files]) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
import os | ||
import platform | ||
|
||
import pytest | ||
|
||
from conans.test.assets.genconanfile import GenConanfile | ||
from conans.test.utils.test_files import temp_folder | ||
from conans.test.utils.tools import TestClient | ||
from conans.util.files import save | ||
|
||
|
||
@pytest.mark.tool("fish") | ||
def test_virtualenv_fish(): | ||
cache_folder = os.path.join(temp_folder(), "[sub] folder") | ||
client = TestClient(cache_folder) | ||
conanfile = str(GenConanfile("pkg", "0.1")) | ||
conanfile += """ | ||
|
||
def package_info(self): | ||
self.buildenv_info.define_path("MYPATH1", "/path/to/ar") | ||
""" | ||
client.save({"conanfile.py": conanfile}) | ||
client.run("create .") | ||
save(client.cache.new_config_path, "tools.env.virtualenv:fish=True\n") | ||
client.save({"conanfile.py": GenConanfile("app", "0.1").with_requires("pkg/0.1")}) | ||
client.run("install . -s:a os=Linux") | ||
|
||
assert not os.path.exists(os.path.join(client.current_folder, "conanbuildenv.sh")) | ||
assert not os.path.exists(os.path.join(client.current_folder, "conanbuildenv.bat")) | ||
assert not os.path.exists(os.path.join(client.current_folder, "conanrunenv.sh")) | ||
assert not os.path.exists(os.path.join(client.current_folder, "conanrunenv.bat")) | ||
|
||
assert os.path.exists(os.path.join(client.current_folder, "conanbuildenv.fish")) | ||
assert not os.path.exists(os.path.join(client.current_folder, "conanrunenv.fish")) | ||
|
||
with open(os.path.join(client.current_folder, "conanbuildenv.fish"), "r") as f: | ||
buildenv = f.read() | ||
assert 'set -gx MYPATH1 "/path/to/ar"' in buildenv | ||
|
||
client.run_command("fish -c 'source conanbuildenv.fish && set'") | ||
assert 'MYPATH1 /path/to/ar' in client.out | ||
|
||
client.run_command("fish -c 'source conanbuildenv.fish && set && deactivate_conanbuildenv && set'") | ||
assert str(client.out).count('MYPATH1 /path/to/ar') == 1 | ||
uilianries marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
@pytest.mark.tool("fish") | ||
@pytest.mark.parametrize("fish_value, path_with_spaces", ([True, False], [False, True], [False, False], [True, True])) | ||
def test_transitive_tool_requires(fish_value, path_with_spaces): | ||
"""Generate a tool require package, which provides and binary and a custom environment variable. | ||
Using fish, the binary should be available in the path, and the environment variable too. | ||
""" | ||
client = TestClient(path_with_spaces=path_with_spaces) | ||
save(client.cache.new_config_path, f"tools.env.virtualenv:fish={fish_value}\n") | ||
|
||
# Generate the tool package with pkg-echo-tool binary that prints the value of LADY env var | ||
cmd_line = "echo ${LADY}" if platform.system() != "Windows" else "echo %LADY%" | ||
conanfile = str(GenConanfile("tool", "0.1.0") | ||
.with_package_file("bin/pkg-echo-tool", cmd_line)) | ||
package_info = """ | ||
os.chmod(os.path.join(self.package_folder, "bin", "pkg-echo-tool"), 0o777) | ||
|
||
def package_info(self): | ||
self.buildenv_info.define("LADY", "Dulcinea-del-Toboso") | ||
""" | ||
conanfile += package_info | ||
client.save({"tool/conanfile.py": conanfile}) | ||
client.run("create tool") | ||
|
||
assert "tool/0.1.0: package(): Packaged 1 file: pkg-echo-tool" in client.out | ||
|
||
# Generate the app package that uses the tool package. It should be able to run the binary and | ||
# access the environment variable as well. | ||
conanfile = str(GenConanfile("app", "0.1.0") | ||
.with_tool_requires("tool/0.1.0") | ||
.with_generator("VirtualBuildEnv")) | ||
build = """ | ||
def build(self): | ||
self.run("pkg-echo-tool", env="conanbuild") | ||
""" | ||
conanfile += build | ||
|
||
client.save({"app/conanfile.py": conanfile}) | ||
client.run("create app") | ||
|
||
assert "Dulcinea-del-Toboso" in client.out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be supporting fish at the moment in Windows subsystems, mostly used externally with
win_bash