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:mod1:mod2:prefix: generalized patchFile to allow non-global effect; works on cmdline / config #18496

Closed
wants to merge 5 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jul 15, 2021

This PR 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.

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 #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: allows patchFile on cmdline and with non-global effect --moduleoverride:mod1:mod2:prefix: generalized patchFile to allow non-global effect; works on cmdline Jul 15, 2021
@timotheecour timotheecour changed the title --moduleoverride:mod1:mod2:prefix: generalized patchFile to allow non-global effect; works on cmdline --moduleoverride:mod1:mod2:prefix: generalized patchFile to allow non-global effect; works on cmdline / config Jul 15, 2021
@juancarlospaco
Copy link
Collaborator

juancarlospaco commented Jul 15, 2021

Interesting, it always looks strange that you can do this for NimScript but not for other targets.
🙂:+1:

@timotheecour timotheecour mentioned this pull request Jul 15, 2021
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Jul 16, 2021
@Varriount
Copy link
Contributor

Varriount commented Jul 16, 2021

I think the needs this mechanism is addressing should be discussed in an RFC first. The compiler command line is already quite complicated, and I think some additional effort should be made to ensure that the amount of any additional complexity, if it is needed, is as small as possible.

@timotheecour
Copy link
Member Author

timotheecour commented Jul 16, 2021

I think the needs this mechanism is addressing should be discussed in an RFC first.

=> filed nim-lang/RFCs#400

The compiler command line is already quite complicated, and I think some additional effort should be made to ensure that the amount of any additional complexity, if it is needed, is as small as possible.

simple things should be simple, complex things should be possible.

--moduleoverride:mod1:mod2:prefix is not something you'd need nor encounter as a beginner, but when the need for it comes, you will find it useful (I certainly found patchFile useful). I can't think of a more succint way to describe the override than with the proposed flag.

@Araq
Copy link
Member

Araq commented Jul 20, 2021

simple things should be simple, complex things should be possible.

Complexity breeds complexity.

@arnetheduck
Copy link
Contributor

example use case 1: breaking change mitigation

I don't see this being a use case really - ie the onus of maintaining compatibility is on the std lib in the case of Nim itself - other depenedencies that can be independently upgraded don't need this kind of facility generally

@timotheecour timotheecour mentioned this pull request Aug 29, 2021
2 tasks
@stale
Copy link

stale bot commented Aug 10, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Aug 10, 2022
@stale stale bot closed this Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review (please take another look): ready for next review round stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants