-
-
Notifications
You must be signed in to change notification settings - Fork 619
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
[core] new cmd path decorations with %[] syntax #769
Conversation
this is for marking cmd options as paths relative to the projects base dir. decorated paths can use tokens / macros decorated paths are wrapped with "". Final slashes are honored. Slashes are platform specific. Note that the working dir for custom cmd is undefined as the current working dir will be different between xcode (wks.location) and visual studio / make (prj.location.) Changing the CWD isn't a good default behavior (for reasons.) This leads to prior use of premake requiring alot of string concating / function calls for path translations which make the cmd line difficult to read and maintain. With path decorations, one can just make those relative paths with %[] and everything should work.
|
||
function suite.translateCommandsAndPaths_PreserveSlash() | ||
test.isequal('cmdtool "../foo/path1/"', os.translateCommandsAndPaths("cmdtool %[path1/]", '../foo', '.', 'osx')) | ||
end |
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.
These tests are great, but I've noticed a bit of a trend of our Lua patterns not working with the same pattern twice. For example, what happens if you were to have cmdtool %[path1/] %[path2/]
?
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.
We can certainly add a test for that... the output should be cmdtool "../foo/path1/" "../foo/path2/"
We use this in several places like that... but adding a test for it is probably a good idea.
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.
Awesome, I can never really tell with the Lua patterns. 👍
--- | ||
|
||
function os.translateCommandsAndPaths(cmds, basedir, location, map) | ||
local translatedBaseDir = path.getrelative(location, basedir) |
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 tend to agree with the code here, just wanted to know your thoughts on having the caller perform this? Would there be any benefit in this function being "flexible"?
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 it matters much, whatever we prefer... it just moves this single line into 5-6 locations instead...
this is for marking cmd options as paths relative to the projects base dir. decorated paths can use tokens / macros decorated paths are wrapped with "". Final slashes are honored. Slashes are platform specific.
You can use '%[path]' in any kind of buildcommands API, to decorate an option as a path that needs to be resolved... Currently there is no reliable way to identify parts of a string as a path, which is what this adds, writing buildcommands is quite a bit easier because of that.