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

--moduleoverride: generalized patchFile; allow overriding a module for select packages/modules only #400

Open
1 task
timotheecour opened this issue Jul 16, 2021 · 5 comments

Comments

@timotheecour
Copy link
Member

This PR nim-lang/Nim#18496 improves and generalizes (and in fact subsumes) patchFile as follows:

the most important aspect of --moduleoverride though is that it allows applying the module override in a non-global way, using module prefixes, eg:

--moduleoverride:std/sequtils:std/sequtilsv0 # applies globally
--moduleoverride:std/sequtils:std/sequtilsv0:pkg/mypkg  # override only applies within pkg/mypkg
--moduleoverride:std/sequtils:std/sequtilsv0:pkg/mypkg/sub # only within pkg/mypkg/sub

prefix is an optional comma delimited set of prefixes, eg:
std/strutils,pkg/mypkg,pkg/cligen/bar

it can also contain absolute paths (but not relative paths; a std/ or system/ or pkg/ prefix with a canonical name must be used if the path is not absolute)

example use case 1: breaking change mitigation

this can be used to override a module for just a package, without affecting the behavior of other packages. For eg, when upgrading nim to a new version, if some package foo requires a pre-existing behavior in some stdlib module (eg std/sequtils), you can override std/sequtils as needed and make the override apply only within package foo, unlike a flag like -d:nimLegacyFoo which has global effect:

nim --moduleoverride:std/sequtils:pkg/foo/sequtils_override:pkg/foo /pathto/main.nim

example use case 2: custom behavior for a dependency

this is useful for experimentation or if you want to change behavior of a 3rd party package foo in a way that doesn't affect other dependencies that also depend on foo.

example use case 3: replacement patchFile that can work on cmdline/cfg/nims files

(unlike patchFile which is only nimscript)

note

the overridden module can co-exist with the non-overridden module within a project, and this ends up being 2 separate modules (PSym).

use --processing:filenames to see what modules are being imported (shows import stacks since nim-lang/Nim#18372)

note: rationale for canonical modules names

the way patchFile refers to module names has inherent ambiguities, eg:

  • duplicate module names within a package eg foo/bar/baz vs foo/baz are confused, both as foo_baz
  • foo_bar/baz vs foo/bar_baz are confused, both as foo_bar_baz

furthermore, it doesn't correspond to how you'd import a module.

future work

  • --symoverride:mod1.sym:sym2:prefix: variant which allows to override a fully qualified symbol mod1.sym by mod1.sym2 within the context of modules matching prefix (eg system.delete => system.deleteV0 to get back old behavior within the context of some pkg/foo only). --moduleoverride is more general (because you can override a whole module as you need) but --symoverride can be easier to use in some cases especially in cases where a behavior for an API was changed and you want the old behavior for a package without affecting other packages.
@timotheecour timotheecour changed the title --moduleoverride:mod1:mod2:prefix: generalized patchFile to allow non-global effect; works on cmdline / config --moduleoverride: generalized patchFile; allow overriding a module for select packages only Jul 16, 2021
@timotheecour timotheecour changed the title --moduleoverride: generalized patchFile; allow overriding a module for select packages only --moduleoverride: generalized patchFile; allow overriding a module for select packages/modules only Jul 16, 2021
@Araq
Copy link
Member

Araq commented Jul 20, 2021

So let's see: The compiler compiles two versions of strutils.nim for the same program. What could go wrong? Every extern: "nsuParseFloat" declaration means we produce 2x the same C code, the compilation fails. And that's the first issue that I could come up with, without testing the feature at all, just running it in my head.

Next problem: The compiler reports an error like "Expected type Foo but got type Foo". So ... it's only a couple of dozen "bugfixes" away from working. Complexity breeds complexity, you cannot fight complexity with "let's have more features".

@timotheecour
Copy link
Member Author

timotheecour commented Jul 20, 2021

I'm aware of this, include has similar issues; the user is in control and can write:

# in strutils2:
func normalize*(s: string): string {.rtl, extern: "strutils2_nsuNormalize".} =

and then it works, i tried.

furthermore, this can be solved by using module name prefix in extern pattern using $m, so that user doesn't need to change anything:

# in strutils2:
func normalize*(s: string): string {.rtl, extern: "$m_nsuNormalize".} =

and a single PR can change all the extern declarations in stdlib accordingly with $m_ instead of hardcoding it via "nsuNormalize"

@c-blake
Copy link

c-blake commented Aug 10, 2021

Araq wrote:

Complexity breeds complexity

Before all this std/ and pkg/ shenanigans, if I wanted to override a stdlib module then I could just do it with --path. Now I need to use patchFile or maybe this new gizmo.

Cannot the simple "override a module by finding it first in path" mechanism still be made to work or is that now impossible to make work again? Maybe a --path-force or --path-std?

@timotheecour
Copy link
Member Author

Before all this std/ and pkg/ shenanigans, if I wanted to override a stdlib module then I could just do it with --path. Now I need to use patchFile or maybe this new gizmo.

Cannot the simple "override a module by finding it first in path" mechanism still be made to work or is that now impossible to make work again? Maybe a --path-force or --path-std?

I don't see how this comment relates to this PR. --path and patchFile still work the same before or after this PR and you can continue to use that; patchFile is not the same as --path but anyways pre-exists to this PR.

Cannot the simple "override a module by finding it first in path" mechanism still be made to work or is that now impossible to make work again?

I don't understand this comment, --path still works. This PR generalizes patchFile and makes it work on cmdline (or config) instead of just nims file, and allows overriding a module for a select package/module only, optionally, which is not something you can do with either --path or patchFile.

@c-blake
Copy link

c-blake commented Aug 11, 2021

I don't understand your not understanding. Maybe @Araq does.

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

No branches or pull requests

3 participants