Issue 29390657: Issue 5020 - Part 2: Fix usage of paths in compile script (Closed)
|
Created:
March 21, 2017, 11:22 a.m. by Wladimir Palant Modified:
March 27, 2017, 7:59 a.m. Reviewers:
Vasily Kuznetsov CC:
hub Base URL:
https://hg.adblockplus.org/adblockpluscore Visibility:
Public. |
DescriptionIssue 5020 - Part 2: Fix usage of paths in compile script
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removed fallback #MessagesTotal messages: 8
https://codereview.adblockplus.org/29390657/diff/29390658/compile File compile (right): https://codereview.adblockplus.org/29390657/diff/29390658/compile#newcode7 compile:7: BASE_DIR = os.path.dirname(__file__) or os.getcwd() BASE_DIR (which I suppose is the path to the directory where this file lives) will be absolute if we run it from that same directory (in this case the left operand of "or" will be an empty string) or relative if we run it from any other directory. Perhaps it would be more consistent to always generate an absolute path: os.path.abspath(os.path.dirname(__file__)) or always use a relative path by omitting "or os.getcwd()". Or am I misunderstanding the intention here?
https://codereview.adblockplus.org/29390657/diff/29390658/compile File compile (right): https://codereview.adblockplus.org/29390657/diff/29390658/compile#newcode7 compile:7: BASE_DIR = os.path.dirname(__file__) or os.getcwd() On 2017/03/21 18:01:42, Vasily Kuznetsov wrote: > BASE_DIR (which I suppose is the path to the directory where this file lives) > will be absolute if we run it from that same directory (in this case the left > operand of "or" will be an empty string) or relative if we run it from any other > directory. Perhaps it would be more consistent to always generate an absolute > path: os.path.abspath(os.path.dirname(__file__)) or always use a relative path > by omitting "or os.getcwd()". > > Or am I misunderstanding the intention here? It's fine to have relative paths for almost all purposes (shell file being an exception which is why we generate an absolute path there). The point is rather that this script *can* be run from a different directory, so ../adblockpluscore/compile will still do the right thing (e.g. find the files to be compiled in ../adblockpluscore/compiled).
Sorry for the persistence, I just want to make sure I understand the context and the reasoning. https://codereview.adblockplus.org/29390657/diff/29390658/compile File compile (right): https://codereview.adblockplus.org/29390657/diff/29390658/compile#newcode7 compile:7: BASE_DIR = os.path.dirname(__file__) or os.getcwd() On 2017/03/22 08:44:20, Wladimir Palant wrote: > On 2017/03/21 18:01:42, Vasily Kuznetsov wrote: > > BASE_DIR (which I suppose is the path to the directory where this file lives) > > will be absolute if we run it from that same directory (in this case the left > > operand of "or" will be an empty string) or relative if we run it from any > other > > directory. Perhaps it would be more consistent to always generate an absolute > > path: os.path.abspath(os.path.dirname(__file__)) or always use a relative path > > by omitting "or os.getcwd()". > > > > Or am I misunderstanding the intention here? > > It's fine to have relative paths for almost all purposes (shell file being an > exception which is why we generate an absolute path there). The point is rather > that this script *can* be run from a different directory, so > ../adblockpluscore/compile will still do the right thing (e.g. find the files to > be compiled in ../adblockpluscore/compiled). I guess I'm missing some part of the picture here. What do you mean by "shell file" and why would that case be an exception and require an absolute path?
https://codereview.adblockplus.org/29390657/diff/29390658/compile File compile (right): https://codereview.adblockplus.org/29390657/diff/29390658/compile#newcode7 compile:7: BASE_DIR = os.path.dirname(__file__) or os.getcwd() On 2017/03/22 09:52:58, Vasily Kuznetsov wrote: > I guess I'm missing some part of the picture here. What do you mean by "shell > file" and why would that case be an exception and require an absolute path? See SHELL_FILE key in GENERATION_PARAMS below. It is an Emscripten parameter and for some reason Emscripten insists that it has to be an absolute path.
https://codereview.adblockplus.org/29390657/diff/29390658/compile File compile (right): https://codereview.adblockplus.org/29390657/diff/29390658/compile#newcode7 compile:7: BASE_DIR = os.path.dirname(__file__) or os.getcwd() On 2017/03/22 14:59:29, Wladimir Palant wrote: > On 2017/03/22 09:52:58, Vasily Kuznetsov wrote: > > I guess I'm missing some part of the picture here. What do you mean by "shell > > file" and why would that case be an exception and require an absolute path? > > See SHELL_FILE key in GENERATION_PARAMS below. It is an Emscripten parameter and > for some reason Emscripten insists that it has to be an absolute path. Ah, ok, now I get it. But then we don't really need "or os.getcwd()" in this line since empty BASE_DIR would be a correct relative path in case this script is run from the directory where it's located. And Emscripten will be happy anyway because there's another os.path.abspath there.
https://codereview.adblockplus.org/29390657/diff/29390658/compile File compile (right): https://codereview.adblockplus.org/29390657/diff/29390658/compile#newcode7 compile:7: BASE_DIR = os.path.dirname(__file__) or os.getcwd() On 2017/03/22 19:16:01, Vasily Kuznetsov wrote: > Ah, ok, now I get it. But then we don't really need "or os.getcwd()" in this > line since empty BASE_DIR would be a correct relative path in case this script > is run from the directory where it's located. Fair enough, I verified that os.path.join('', 'foo') will do the right thing - I wasn't aware of that. Removed the fallback.
LGTM 
|
