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

fish shares universal variables between architectures #2329

Closed
andrew-schulman opened this issue Aug 24, 2015 · 3 comments
Closed

fish shares universal variables between architectures #2329

andrew-schulman opened this issue Aug 24, 2015 · 3 comments

Comments

@andrew-schulman
Copy link
Contributor

@andrew-schulman andrew-schulman commented Aug 24, 2015

In fish 2.2.0 in Cygwin, shells running in the i686 and x86_64 architectures share universal variables, since they read and write them to the same file, ~/.config/fish/fishd.$HOSTNAME.

In my opinion this is undesirable, because environment variables can contain architecture-specific information. For example, in x86_64 PATH may include directories with 64- and 32-bit executables in them, but in i686 this will fail.

To avoid this problem we currently use the following patch:

diff -urN fish-2.2.0.orig/env_universal_common.cpp fish-2.2.0/env_universal_common.cpp
--- fish-2.2.0.orig/env_universal_common.cpp    2015-07-03 15:46:59.000000000 -0400
+++ fish-2.2.0/env_universal_common.cpp 2015-08-22 03:55:44.925764700 -0400
@@ -84,6 +84,13 @@
     wcstring result = wdir;
     result.append(L"/fishd.");
     result.append(get_machine_identifier());
+#if defined(__CYGWIN__)
+#if defined(__x86_64__)
+    result.append(L".x86_64");
+#elif defined(__i686__)
+    result.append(L".i686");
+#endif
+#endif
     return result;
 }

which just appends the architecture name to the variables filename.

Note that prior to 2.2.0, fish automatically didn't share variables between architectures, because it ran separate copies of fishd in separate file systems. This worked fine, and as the Cygwin maintainer I never heard any complaints about it.

If other OSes also run on multiple architectures, then it might be desirable to always append the architecture name to the variables file name, using uname().

Please label this issue as windows.

@ridiculousfish ridiculousfish added this to the fish-future milestone Aug 24, 2015
@zanchey
Copy link
Member

@zanchey zanchey commented Aug 26, 2015

Windows is the best. Can you really not invoke a 64-bit process from a 32-bit process?

@andrew-schulman
Copy link
Contributor Author

@andrew-schulman andrew-schulman commented Aug 31, 2015

Hm, ok. I tried that, and actually you can launch a 64-bit process from a 32-bit process.

So at the moment I don't have any examples of problems that could come from sharing variables between architectures. It still seems like a bad idea to me, but I guess I need to think about it.

@zanchey
Copy link
Member

@zanchey zanchey commented Sep 4, 2015

I can't think of anything either, so I might close this for now - but do speak up if you think of an example!

@faho faho added the invalid label Oct 24, 2015
@faho faho removed this from the fish-future milestone Oct 24, 2015
@krader1961 krader1961 modified the milestone: will-not-implement Nov 22, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants