make config.nims behave like nim.cfg in terms of where these scripts are searched / run #8682

timotheecour · 2018-08-18T08:12:45Z

/cc @Araq

run $systemConfigPath/config.nims if it exists
run ~/.config/nim/config.nims if it exists
run parendDir/config.nims if exists for each parendDir
run project config.nims if exists
run inputfile.nims if exists

these are controlled by `optSkipConfigFile

makes config.nims behave like nim.cfg
supersides [superseded] partially address issue #8217 by running user config.nims if possible #8233
fixes: config.nims should apply to subdirectories recursively, like nim.cfg does config.nims should apply to subdirectories recursively, like nim.cfg does #8217
refactors duplicate/out of sync code between nim.nim and nimsuggest.nim

timotheecour · 2018-08-20T17:59:45Z

friendly ping

alaviss · 2018-08-21T12:20:05Z

Can you make it run system config.nims as well?

timotheecour · 2018-08-23T23:31:14Z

@alaviss

Can you make it run system config.nims as well?

done

PTAL

Araq

Look at my remarks.

Araq · 2018-08-25T20:37:59Z

compiler/nim.nim

-    if fileExists(scriptFile):
-      runNimScript(cache, scriptFile, freshDefines=false, conf)
+
+    proc runNimScriptIfExists(scriptFile:string)=


Space after the colon: Just like in written English.

Araq · 2018-08-25T20:38:43Z

compiler/nim.nim

+    # merge this complex logic with `loadConfigs`
+    # check whether these should be controlled via
+    # optSkipConfigFile, optSkipUserConfigFile
+    let config_nims = "config.nims"


That's a const and the Nim compiler doesn't use under_scores.

Araq · 2018-08-25T20:41:02Z

compiler/nim.nim

-    elif fileExists(conf.projectPath / "config.nims"):
-      # directory wide NimScript file
-      runNimScript(cache, conf.projectPath / "config.nims", freshDefines=false, conf)
+      if fileExists(scriptFile) and scriptFile == conf.projectFull: return


Why does this have a fileExists check? If the same as conf.projectFull do return. And this should probably use cmpPaths...

Why does this have a fileExists check? If the same as conf.projectFull do return

it's needed; with your suggestion, nim c foo.nims would exit without error when foo.nims doesn't exist; with this PR (and in fact preexisting to this PR), it would give an error

And this should probably use cmpPaths

done

timotheecour · 2018-08-25T21:48:04Z

PTAL

timotheecour · 2018-08-27T02:24:44Z

@Araq friendly ping

Araq · 2018-08-27T06:32:35Z

Sorry, one more thing to do: Patch nimsuggest so that it uses the same configuration file processing.

timotheecour · 2018-08-28T02:54:03Z

/cc @Araq PTAL

Sorry, one more thing to do: Patch nimsuggest so that it uses the same configuration file processing.

done: did a refactoring using OOP, which removed a lot of duplicate (and out of sync!) code.
using OOP in this case is the cleanest solution IMO (avoiding as much as possible if/then/else spagetti code); it also unveiled a few things that were already out of sync in nimsuggest (eg handling of conf.projectName == "-" etc), which I fixed.

after this PR, it will also unblock #8233 which was blocking on your comment here

Good catch. But I cannot merge a PR that makes things worse, then nimsuggest disagrees with nim's way of reading the configuration.

In future PR, if needed, nimfix can also reuse cmdlinehelper to avoid being out of sync (it's been out of sync for some time; I'd rather not do the change in this PR as it's already out of sync with compiler/nim.nim)

timotheecour · 2018-08-28T03:04:26Z

compiler/nim.nim

 proc handleCmdLine(cache: IdentCache; conf: ConfigRef) =
-  condsyms.initDefines(conf.symbols)
+  let self = ProgNim(name: "nim_compiler")


NOTE: this PR adds
defineSymbol conf.symbols, "nim_compiler"
for symmetry with what's done in numsuggest and nimfix which add "numsuggest" and "nimfix" respectively

timotheecour · 2018-08-28T03:06:15Z

compiler/cmdlinehelper.nim

+
+proc processCmdLineAndProjectPath*(self: ProgBase, conf: ConfigRef) =
+  self.processCmdLine(passCmd1, "", conf)
+  if conf.projectName == "-":


this branch was missing prior to this PR for nimsuggest

And yet ... nimsuggest does not support to read the main file from stdin!

timotheecour · 2018-08-28T03:06:35Z

compiler/cmdlinehelper.nim

+    except OSError:
+      conf.projectFull = conf.projectName
+    let p = splitFile(conf.projectFull)
+    let dir = if p.dir.len > 0: p.dir else: getCurrentDir()


this was missing prior to this PR for nimsuggest

timotheecour · 2018-08-28T03:13:33Z

compiler/cmdlinehelper.nim

+      # 'nim foo.nims' means to just run the NimScript file and do nothing more:
+      if fileExists(scriptFile) and scriptFile.cmpPaths(conf.projectFull) == 0:
+        return false
+    else:


a bit more complex that I'd hope, but that's to keep semantics from before this PR

Araq

This is also hard to review because of the large diffs, who knows what features/changes were made. Maybe split it up in two PRs, one that only adds these new place where to hide config files, one that refactors the code duplication.

Araq · 2018-08-28T06:25:38Z

compiler/cmdlinehelper.nim

+    if fileExists(scriptFile):
+      runNimScript(cache, scriptFile, freshDefines=false, conf)
+
+  # TODO:


Don't write what's left TODO we have enough of this in the compiler, do it.

Araq · 2018-08-28T06:28:00Z

compiler/cmdlinehelper.nim

+  condsyms.initDefines(conf.symbols)
+  defineSymbol conf.symbols, self.name
+
+proc processCmdLineAndProjectPath*(self: ProgBase, conf: ConfigRef) =


Instead of this OOP approach, give processCmdLineAndProjectPath two callbacks or - probably better- make it a template.

done with 2 callbacks

Araq · 2018-08-28T06:29:06Z

compiler/nim.nim

@@ -37,7 +37,9 @@ proc prependCurDir(f: string): string =
  else:
    result = f

-proc processCmdLine(pass: TCmdLinePass, cmd: string; config: ConfigRef) =
+type ProgNim=ref object of ProgBase


Empty subclass means you don't need any inheritance/OOP here.

done, changed it to not use OOP

Araq · 2018-08-28T06:29:48Z

nimsuggest/nimsuggest.nim

@@ -487,7 +487,9 @@ proc mainThread(graph: ModuleGraph) =
 var
  inputThread: Thread[ThreadParams]

-proc mainCommand(graph: ModuleGraph) =
+type ProgNimsuggest=ref object of ProgBase


Empty subclass means you don't need any inheritance/OOP here.

BTW Spaces around the '='. Just like you saw it everywhere else in Nim's whole codebase.

timotheecour · 2018-08-29T00:21:39Z

/cc @Araq
PTAL, all comments addressed

This is also hard to review because of the large diffs, who knows what features/changes were made. Maybe split it up in two PRs, one that only adds these new place where to hide config files, one that refactors the code duplication.

I kept different commits at strategic points and for that purpose, you can select the commit range in review, see image

one that only adds these new place where to hide config files

https://github.com/nim-lang/Nim/pull/8682/files/b76d7f1a916b080a69c588c5675afde7575d2445 (1st 4 commits)

one that refactors the code duplication.

https://github.com/nim-lang/Nim/pull/8682/files/b76d7f1a916b080a69c588c5675afde7575d2445..be30af0ccc960a7e6c1a77d9093e1327beef8ef3 (last 2 commits)

alternatively, this also helps: git fetch origin pull/8682/head:8682 && git checkout 8682 && tig

Araq · 2018-08-29T09:43:41Z

compiler/cmdlinehelper.nim

+  std/os
+
+type NimProg* = ref object
+  name*: string


Remove this field and pass name as a parameter to initDefinesProg instead.

Araq · 2018-08-29T09:44:14Z

compiler/cmdlinehelper.nim

+  compiler/[options, idents, nimconf, scriptconfig, extccomp, commands, msgs, lineinfos, modulegraphs, condsyms],
+  std/os
+
+type NimProg* = ref object


type NimProg* = ref object

timotheecour · 2018-08-29T10:31:10Z

/cc @Araq PTAL

timotheecour · 2018-08-29T11:42:50Z

~~NOTE: rebased against #8805 ; when looking at this PR make sure to select appropriate commits in diff view~~ => ok, that other PR was now merged, this is good to go

timotheecour · 2018-08-29T19:49:34Z

/cc @Araq PTAL

kaushalmodi · 2018-10-16T13:47:01Z

@timotheecour Just wanted to thank you heartily for this PR :D

Related: nim-lang#8682

Related: nim-lang#8682 Also mention the "nim help" command to list all available tasks.

timotheecour · 2018-10-16T18:09:08Z

glad to hear, thanks :-)

Related: #8682 Also mention the "nim help" command to list all available tasks.

Related: nim-lang#8682 Also mention the "nim help" command to list all available tasks.

Related: #8682 Also mention the "nim help" command to list all available tasks.

Related: #8682 Also mention the "nim help" command to list all available tasks. (cherry picked from commit 3e9f506)

timotheecour mentioned this pull request Aug 18, 2018

[superseded] run project config.nims if exists, then inputfile.nims if exists #8234

Closed

timotheecour force-pushed the pr_user_config_nims_works branch from e1da01a to c3423fd Compare August 23, 2018 23:30

timotheecour force-pushed the pr_user_config_nims_works branch from c3423fd to 9f1283c Compare August 23, 2018 23:59

timotheecour mentioned this pull request Aug 24, 2018

[travis] [async] flaky test: tests/async/t7758.nim #8756

Closed

timotheecour closed this Aug 24, 2018

timotheecour reopened this Aug 24, 2018

Araq reviewed Aug 25, 2018

View reviewed changes

timotheecour force-pushed the pr_user_config_nims_works branch from 9f1283c to 7b8e420 Compare August 25, 2018 21:41

timotheecour force-pushed the pr_user_config_nims_works branch from 7b8e420 to 3b8caa8 Compare August 28, 2018 02:45

timotheecour mentioned this pull request Aug 28, 2018

[superseded] partially address issue #8217 by running user config.nims if possible #8233

Closed

timotheecour force-pushed the pr_user_config_nims_works branch from 3b8caa8 to 59184ca Compare August 28, 2018 02:59

timotheecour commented Aug 28, 2018

View reviewed changes

Araq reviewed Aug 28, 2018

View reviewed changes

timotheecour force-pushed the pr_user_config_nims_works branch 2 times, most recently from 8b8f445 to be30af0 Compare August 29, 2018 00:05

timotheecour changed the title ~~~/.config/nim/config.nims can now be used + honor project config.nims~~ make config.nims behave like nim.cfg in terms of where these scripts are searched / run Aug 29, 2018

Araq reviewed Aug 29, 2018

View reviewed changes

timotheecour force-pushed the pr_user_config_nims_works branch from 3072a93 to facf529 Compare August 29, 2018 11:41

timotheecour added 7 commits August 29, 2018 12:47

run project config.nims if exists, then inputfile.nims if exists

fd78203

~/.config/nim/config.nims can now be used

970740c

also check in getSystemConfigPath for config.nims

58bffca

address comments

2acd322

refactor handleCmdLine for nim and nimsuggest

58654eb

address comments

57fbb3b

address comments

3ad8f80

timotheecour force-pushed the pr_user_config_nims_works branch from facf529 to 3ad8f80 Compare August 29, 2018 19:48

Araq merged commit ed0cb7b into nim-lang:devel Aug 30, 2018

timotheecour deleted the pr_user_config_nims_works branch August 30, 2018 17:42

kaushalmodi added a commit to kaushalmodi/Nim that referenced this pull request Oct 16, 2018

Update config.nims search path in docs

dab7566

Related: nim-lang#8682

kaushalmodi mentioned this pull request Oct 16, 2018

Update config.nims search path in docs #9388

Merged

kaushalmodi added a commit to kaushalmodi/Nim that referenced this pull request Oct 16, 2018

Update config.nims search path in docs

f91090e

Related: nim-lang#8682 Also mention the "nim help" command to list all available tasks.

Araq pushed a commit that referenced this pull request Oct 16, 2018

Update config.nims search path in docs (#9388)

3e9f506

Related: #8682 Also mention the "nim help" command to list all available tasks.

narimiran pushed a commit to narimiran/Nim that referenced this pull request Oct 31, 2018

Update config.nims search path in docs (nim-lang#9388)

890a756

Related: nim-lang#8682 Also mention the "nim help" command to list all available tasks.

narimiran pushed a commit to narimiran/Nim that referenced this pull request Nov 1, 2018

Update config.nims search path in docs (nim-lang#9388)

0cd6821

Related: nim-lang#8682 Also mention the "nim help" command to list all available tasks.

narimiran pushed a commit that referenced this pull request Nov 1, 2018

Update config.nims search path in docs (#9388)

983b9f4

Related: #8682 Also mention the "nim help" command to list all available tasks.

narimiran pushed a commit that referenced this pull request Nov 1, 2018

Update config.nims search path in docs (#9388)

7e1d411

Related: #8682 Also mention the "nim help" command to list all available tasks. (cherry picked from commit 3e9f506)

make config.nims behave like nim.cfg in terms of where these scripts are searched / run #8682

make config.nims behave like nim.cfg in terms of where these scripts are searched / run #8682

Conversation

timotheecour commented Aug 18, 2018 • edited Loading

timotheecour commented Aug 20, 2018 • edited Loading

alaviss commented Aug 21, 2018

timotheecour commented Aug 23, 2018

Araq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour Aug 25, 2018 • edited Loading

Choose a reason for hiding this comment

timotheecour commented Aug 25, 2018

timotheecour commented Aug 27, 2018

Araq commented Aug 27, 2018

timotheecour commented Aug 28, 2018 • edited Loading

timotheecour Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Araq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Aug 29, 2018

timotheecour commented Aug 29, 2018 • edited Loading

timotheecour commented Aug 29, 2018

kaushalmodi commented Oct 16, 2018

timotheecour commented Oct 16, 2018

timotheecour commented Aug 18, 2018 •

edited

Loading

timotheecour commented Aug 20, 2018 •

edited

Loading

timotheecour Aug 25, 2018 •

edited

Loading

timotheecour commented Aug 28, 2018 •

edited

Loading

timotheecour Aug 28, 2018 •

edited

Loading

timotheecour commented Aug 29, 2018 •

edited

Loading

timotheecour commented Aug 29, 2018 •

edited

Loading