-
Notifications
You must be signed in to change notification settings - Fork 39
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
backend: fix inconsistencies in --genScript:on
mode
#802
Conversation
Thanks for the PR. |
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.
Please update the PR message, it's used as the final commit message the pre-existing PR template walks you through it.
I took a quick look at the code and it seems it compilePattern
, and possibly includeCmd
, could be made let
. The mutable variable and awkward/error prone logic is facilitated by the mutable variable.
Done, although the description is kinda short.
I don't see how |
A small request regarding the summary: could you reword it so that it reflects the bugfix nature of the PR? The current wording makes it sound more like a feature addition. It's not directly mentioned in the template, but since the PR message is used as the final commit message, please also make sure that the lines have a maximum length of 72 characters.
At least for let compilePattern =
if noAbsolutePaths(conf): joinPath(conf.cCompilerPath, exe)
else: exe |
0633623
to
7be10ec
Compare
compiler/backend/extccomp.nim
Outdated
@@ -580,17 +580,16 @@ proc getCompileCFileCmd*(conf: ConfigRef; cfile: Cfile, | |||
options.add ' ' | |||
options.add cfile.customArgs | |||
|
|||
var compilePattern: string | |||
let compilePattern = | |||
if noAbsolutePaths(conf): exe |
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.
Just confirming my understand: this is in fact the crux of the change because regardless of flags the pattern is configured in the same fashion?
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.
getCompilerExe(conf, c, cfile.cname)
is a fallback, but it's currently forced in --genScript:on
mode
7be10ec
to
d220062
Compare
getCompileCFileCmd
thanks again for the PR, @CyberTailor. The PR is used as a fairly detailed change log in order to:
All of these require clear PR messages that describe:
I've pulled together a todo for things I've noticed:
N.B. Just in case it helps: my approach to writing PR message is I brain dump everything under |
getCompileCFileCmd
--genScript:on
mode
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.
That's a much improved description, thanks
/merge |
Merge requested by: @saem Contents after the first section break of the PR description has been removed and preserved below: |
Summary
Bugfix: Make compiler executable configurable in
--genScript:on
mode.
That means you can now use command-line options like
--gcc.exe
ornim.cfg options like
clang.exe
with--genScript:on
flag, and theywill be respected.
This is needed if compiler on the target machine (when
cross-compiling) is named differently than the default.
Change: Use
--cincludes
option in--genScript:on
mode.Since
cIncludes
paths can be specified only via cli, there should beno unintended side effects. This change is made for consistency.
Details
This PR changes behavior of the
getCompileCFileCmd
function in theextccomp
module, in particular:getConfigVar(conf, c, ".exe")
always takes precedence overgetCompilerExe
, even when relative paths are requested.defined compilers used format strings in the executable.