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

fix #13633 fix koch boot crashing regression #13635

Merged
merged 1 commit into from
Mar 12, 2020

Conversation

timotheecour
Copy link
Member

#13606 used the same refactoring pattern in 2 places, it turns out one introduced a regression, and one introduced by accident a bugifx to HCR IIUC:

accidental regression

fixed by this PR:

accidental bugfix

  • by accident fixed a bug that's been there probably forever
var currIdx = 0	
  for it in list:	
    # call the C compiler for the .c file:	
    if it.flags.contains(CfileFlag.Cached): continue	
    var compileCmd = getCompileCFileCmd(conf, it, currIdx == list.len - 1, produceOutput=true)

=> 

for idx, it in conf.toCompile:
    # call the C compiler for the .c file:
    if CfileFlag.Cached in it.flags: continue
    let compileCmd = getCompileCFileCmd(conf, it, idx == conf.toCompile.len - 1, produceOutput=true)

so before #13606, when at least one file was cached (if it.flags.contains(CfileFlag.Cached): continue ), currIdx == list.len - 1 would be always false, so isMainFile would be false even for the main module in getCompileCFileCmd, wrongly adding -fpic for the main module:

  if (optGenDynLib in conf.globalOptions or (conf.hcrOn and not isMainFile)) and
      ospNeedsPIC in platform.OS[conf.target.targetOS].props:
    options.add(' ' & CC[c].pic)

timotheecour referenced this pull request Mar 12, 2020
@Clyybber
Copy link
Contributor

Clyybber commented Mar 12, 2020

by accident fixed a bug that's been there probably forever

That wasn't by accident :)

Seems like I was a tad bit quicker here :p

@Clyybber Clyybber closed this Mar 12, 2020
@timotheecour
Copy link
Member Author

timotheecour commented Mar 12, 2020

That wasn't by accident :)

but I didn't see it mentioned in your PR, this should definitely be mentioned ; maybe add it to your merged PR that this fixes XYZ, maybe some old obscure HCR bug got unfixed, who knows

@Clyybber
Copy link
Contributor

Yeah, it fixes a HCR bug. (at least thats where I saw it surface once)

@timotheecour
Copy link
Member Author

also, this PR that you closed was fixing another bug, but I dont' have rights to re-open my own PR so I'll open a new one...

@Clyybber
Copy link
Contributor

Oh, sorry. I can reopen if you want to? Its the \n typo right?

@Clyybber Clyybber reopened this Mar 12, 2020
@timotheecour
Copy link
Member Author

ya; I've now rebased...

@Araq Araq merged commit 60a3e03 into nim-lang:devel Mar 12, 2020
@timotheecour timotheecour deleted the pr_fix_13633_JSON_koch_boot branch March 12, 2020 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

koch boot fails if even an empty config.nims is present in ~/.config/nims/ [devel regression]
3 participants