You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by GitBox <gi...@apache.org> on 2021/02/09 08:08:05 UTC
[GitHub] [buildstream] abderrahim opened a new pull request #1440: _context.py: make directories in the configuration absolute
abderrahim opened a new pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440
bst doesn't check that the paths set are absolute and sometimes breaks if
they aren't
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] juergbi commented on a change in pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
juergbi commented on a change in pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440#discussion_r578990307
##########
File path: src/buildstream/_context.py
##########
@@ -282,6 +282,7 @@ def load(self, config=None):
path = os.path.expanduser(path)
path = os.path.expandvars(path)
path = os.path.normpath(path)
+ path = os.path.abspath(path)
Review comment:
I think this could be rather confusing without an explicit opt-in. If absolute paths in the config file are inconvenient for CI, could you set the `XDG_CACHE_HOME` environment variable instead to relocate BuildStream cache files?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim closed pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
abderrahim closed pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] gtristan commented on a change in pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
gtristan commented on a change in pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440#discussion_r579012757
##########
File path: src/buildstream/_context.py
##########
@@ -282,6 +282,7 @@ def load(self, config=None):
path = os.path.expanduser(path)
path = os.path.expandvars(path)
path = os.path.normpath(path)
+ path = os.path.abspath(path)
Review comment:
I am also reluctant to allow relative paths in user configuration.
In the case that user configuration is used to store user preferences, having BuildStream behave differently depending on what your CWD might be at the time of invocation is undesirable, I much prefer it be impossible for the user to accidentally hurt themselves in this way.
If the user configuration is ephemeral, e.g. it is automatically generated as a part of some CI, then it will be fine to feed in full paths anyway.
Further, @juergbi points out the possibility of using `XDG_CACHE_HOME` instead of the user config to relocate the directories, but I should also point out that the directories in user config will expand any environment variables (so you can even specify your own custom env vars to control these).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on a change in pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
abderrahim commented on a change in pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440#discussion_r579633757
##########
File path: src/buildstream/_context.py
##########
@@ -282,6 +282,7 @@ def load(self, config=None):
path = os.path.expanduser(path)
path = os.path.expandvars(path)
path = os.path.normpath(path)
+ path = os.path.abspath(path)
Review comment:
Setting `XDG_CACHE_HOME` is what we used to do, and we encountered a bug in Gitlab CI that didn't correctly export the variables.
The config is was partly generated automatically, partly committed to the repository. But I think it's better to commit it to the repo, which is what we're currently doing.
Since the config expands any environment variable, it can also expand ${PWD} (just tested with 1.6.1 and it does). Do you think this can be considered "an explicit opt-in"?
One more: do you think it's reasonable to add the absolute path check to bst-1? Currently relative paths are accepted, but only work for logdir – the others print BUG messages at some point.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] juergbi commented on a change in pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
juergbi commented on a change in pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440#discussion_r582703027
##########
File path: src/buildstream/_context.py
##########
@@ -282,6 +282,7 @@ def load(self, config=None):
path = os.path.expanduser(path)
path = os.path.expandvars(path)
path = os.path.normpath(path)
+ path = os.path.abspath(path)
Review comment:
Can we close this as `${PWD}` is already available or is still something missing in master?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on a change in pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
abderrahim commented on a change in pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440#discussion_r572721875
##########
File path: src/buildstream/_context.py
##########
@@ -282,6 +282,7 @@ def load(self, config=None):
path = os.path.expanduser(path)
path = os.path.expandvars(path)
path = os.path.normpath(path)
+ path = os.path.abspath(path)
Review comment:
Actually, this is a forward port of #1441. bst-1 accepts relative paths at this point, but fails in funny ways later on. If this is not supported in bst-2 (something I didn't test), we should probably backport that change instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on a change in pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
abderrahim commented on a change in pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440#discussion_r583113895
##########
File path: src/buildstream/_context.py
##########
@@ -282,6 +282,7 @@ def load(self, config=None):
path = os.path.expanduser(path)
path = os.path.expandvars(path)
path = os.path.normpath(path)
+ path = os.path.abspath(path)
Review comment:
In hindsight, what I wanted is not having things in the current directory, but in the root directory of the project. I still think it would be nice to have something like a `%{project-root}` variable that can be used in the configuration, but that's a different issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] juergbi commented on a change in pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
juergbi commented on a change in pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440#discussion_r572679295
##########
File path: src/buildstream/_context.py
##########
@@ -282,6 +282,7 @@ def load(self, config=None):
path = os.path.expanduser(path)
path = os.path.expandvars(path)
path = os.path.normpath(path)
+ path = os.path.abspath(path)
Review comment:
We already reject relative paths (except for the special case with `workspacedir`), see the lines below your change. And we want to reject them, not make them absolute. Otherwise you get different behavior depending on your working directory, which doesn't make sense in this case.
Is the `isabs()` check below not working as it should or what is the motivation for this change?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] juergbi commented on a change in pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
juergbi commented on a change in pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440#discussion_r579640558
##########
File path: src/buildstream/_context.py
##########
@@ -282,6 +282,7 @@ def load(self, config=None):
path = os.path.expanduser(path)
path = os.path.expandvars(path)
path = os.path.normpath(path)
+ path = os.path.abspath(path)
Review comment:
`${PWD}` is sufficiently explicit, in my opinion. I wouldn't recommend this to regular users but if it's useful for CI, sure, use it.
Backporting commit f2c3f8eb0db188caac6991632c4453c39b42e84c to bst-1 sounds reasonable to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream] abderrahim commented on a change in pull request #1440: _context.py: make directories in the configuration absolute
Posted by GitBox <gi...@apache.org>.
abderrahim commented on a change in pull request #1440:
URL: https://github.com/apache/buildstream/pull/1440#discussion_r578973807
##########
File path: src/buildstream/_context.py
##########
@@ -282,6 +282,7 @@ def load(self, config=None):
path = os.path.expanduser(path)
path = os.path.expandvars(path)
path = os.path.normpath(path)
+ path = os.path.abspath(path)
Review comment:
So the reason for wanting this is to have buildstream store its files inside the current directory (for instance in CI) rather than storing them in the user directory.
I think this is something reasonable to have, isn't it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org