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

instead of copyDir(fustion, stdlib/lib), update --path #25

Closed
timotheecour opened this issue Oct 11, 2020 · 12 comments · Fixed by nim-lang/Nim#16925
Closed

instead of copyDir(fustion, stdlib/lib), update --path #25

timotheecour opened this issue Oct 11, 2020 · 12 comments · Fixed by nim-lang/Nim#16925

Comments

@timotheecour
Copy link
Member

@alaviss
continuation of #22 (comment) to avoid mingling unrelated topics

instead of copyDir(distDir / "fusion" / "src" / "fusion", "lib" / "fusion"), we should probably use the usual nimble logic which allows adding import dirs to nim's --path, so that nimble develop fusion works as intended (NIMBLE_DIR=customdir can always be used during bootstrap to avoid user's environment messing up with bootstrap)

I decided against this and make use of copyDir simply because nimble does not support "system-installed" dependencies. Also it's widely incompatible with how Linux distributions package Nim, so unless we got those sorted out copying is the foolproof way.

  • wouldn't NIMBLE_DIR=pathto/Nim/dist work?
  • if not, why not add --path:dist/fusion/src to Nim/config/nim.cfg ?
    if the sources are there it'll pick them up, else, it will be a noop
@alaviss
Copy link
Contributor

alaviss commented Oct 11, 2020

  • wouldn't NIMBLE_DIR=pathto/Nim/dist work?

NIMBLE_DIR can only have one directory at a time, and it's meant as an override to be issued by the user. Set it and --NimblePath will be ignored.

  • if not, why not add --path:dist/fusion/src to Nim/config/nim.cfg ?

Because Nim packaging is currently semi-compatible with the FHS and I do not want to make it worse. Adding paths relative to $nim is dangerous since Linux distributions do not package Nim in the same way as the official binary packages does (in fact the recent requirement of $nim/doc/nimdoc.css for docs has already caused issues with distributions). I do not want to break all distributions this close to release (and it would require discussions anyway due to how $nim is splitted into many different locations in an FHS-compatible installation layout), so I opted for the simpler method of just copying things over.

In the future I'll write an RFC for a formal file system layout for Nim so that additions like this can be simplified and maybe we can then use $nim reliably.

@timotheecour
Copy link
Member Author

how does that interact with nimble install fusion (or nimble develop https://github.com/nim-lang/fusion with some local modifications)?

eg, after ./koch tools, fusion is added to lib so would take precedence over nimble path where user installed fusion?
If so that's not good

@alaviss
Copy link
Contributor

alaviss commented Oct 12, 2020

As usual, user paths come first. If you want to use std fusion, import std / fusion, otherwise, import pkg / fusion, at least that's how I interpret Nim's special pkg, std prefixes.

@timotheecour
Copy link
Member Author

timotheecour commented Oct 12, 2020

  • so an un-qualified import fusion/foo would use pkg/fusion/foo instead of std/fusion/foo, and error if pkg/fusion/foo is not found (and not try to search under std/fusion/foo) ?

  • what happens when a project imports both std/fusion/bar and pkg/fusion/bar (even if those imports are imported from different modules, say via a third party pacakge) when a exportc symbol is defined, wouldn't that cause duplicate defined symbol link error?

this might need to be thought through

@alaviss
Copy link
Contributor

alaviss commented Oct 12, 2020

  • so an un-qualified import fusion/foo would use pkg/fusion/foo instead of std/fusion/foo, and error if pkg/fusion/foo is not found (and not try to search under std/fusion/foo) ?

The search will work like every other stdlib things... If user installed version is not found, use stdlib.

  • what happens when a project imports both std/fusion/bar and pkg/fusion/bar (even if those imports are imported from different modules, say via a third party pacakge) when a exportc symbol is defined, wouldn't that cause duplicate defined symbol link error?

Yes, as usual. All of these concerns apply for basically any other stdlib stuff. fusion is nothing special here.

If we want to employ nimble path for fusion, we would still need support within nimble itself, or user's packages won't build when they use fusion because nimble will clear all paths, and if you add fusion to requires nimble would not be aware of the system-provided fusion and proceed to clone fusion from here instead.

@timotheecour
Copy link
Member Author

As usual, user paths come first. If you want to use std fusion, import std / fusion, otherwise, import pkg / fusion, at least that's how I interpret Nim's special pkg, std prefixes.

I think that's a recipie for a lot of problems down the road, for several reasons:

  • importc procs with duplicate definitions for std/fusion/foo and pkg/fusion/foo
  • std/fusion/foo calling some code that calls import fusion/bar pointing to pkg/fusion/bar instead of the intended std/fusion/bar

why not instead the following:

fusion is default installed (with either --path adjustment preferably or file copies if absolutely necessary) in such a way that:

  • import std/fusion/foo should not compile (because there wouldn't be anything there)
  • import fusion/foo would resolve to pkg/fusion/foo if fusion was installed via nimble install/develop, else would resolve to the bundled fusion installed with nim
  • import pkg/fusion/foo would only resolve to fusion installed via nimble install/develop, and not resolve to bundled fusion installed with nim (that last rule could be optional, and is secondary in this proposal)

this guarantees that there's only fusion used; in particular in the common case where user has installed fusion via nimble install fusion, there would be no clash.
In the rare case where you need 2 distinct fusion dirs for the same project, you can use explicit absolute import paths or we can discuss further for options.

@alaviss
Copy link
Contributor

alaviss commented Oct 15, 2020

  • std/fusion/foo calling some code that calls import fusion/bar pointing to pkg/fusion/bar instead of the intended std/fusion/bar

This rarely happens, and all it would take to solve this is to practice proper import hygiene within the project, as with all nimble packages.

  • import fusion/foo would resolve to pkg/fusion/foo if fusion was installed via nimble install/develop, else would resolve to the bundled fusion installed with nim

  • import pkg/fusion/foo would only resolve to fusion installed via nimble install/develop, and not resolve to bundled fusion installed with nim (that last rule could be optional, and is secondary in this proposal)

This is too magical. I imagine fusion to be a bundled nimble package. It should not receive any special import treatment. This is currently not the case, but once nimble have support for multiple nimble directory we can implement fusion in that direction.

@timotheecour
Copy link
Member Author

timotheecour commented Oct 15, 2020

This rarely happens, and all it would take to solve this is to practice proper import hygiene within the project, as with all nimble packages.

it's not so rare, I've seen it many times, eg:

jester/request.nim:3:1:import jester/private/utils
lib/pure/net.nim:77:1:import std/private/since

The problem is the current import semantics are not ideal for relative paths, eg:

import mypkg/foo # likely incorrect if the importing file is within mypkg (violates import hygiene)
import ./[foo,bar] # not supported (it's a wontfix github issue IIRC)
import "."/[foo,bar] # uglier
import foo, bar
  # big downside: impossible to tell from reading this source file whether this points to a relative path or uses pkg/foo/bar
  # this is also a source of bugs when things move around, with refactorings etc

import ../../path/to/baz # this is hard to write, and brittle if things move around

I'd be like to propose the following import syntax which would solve those issues:

# example:
import this/sub/foo # this resolves to /pathto/fusion/src/fusion/sub/foo.nim anywhere inside /pathto/fusion/ where /pathto/fusion/fusion.nimble is defined
  • avoids need for ../ in relative import paths
  • avoids above mentioned ambiguities
  • more closely matches the import syntax in external packages, eg just replace import this/sub/foo by import fusion/sub/foo, no need to do relative path guesswork

This is currently not the case, but once nimble have support for multiple nimble directory we can implement fusion in that direction.

can you elaborate on this or give a link where multiple nimble directory is discussed?

@alaviss
Copy link
Contributor

alaviss commented Oct 15, 2020

I'd be like to propose the following import syntax which would solve those issues:

I wouldn't be against that but it appears to be unrelated to fusion and more about nimble packages instead. I assume that you will formulate a proper RFC for the nim-lang/RFCs repo?

can you elaborate on this or give a link where multiple nimble directory is discussed?

nim-lang/nimble#80

@timotheecour
Copy link
Member Author

I assume that you will formulate a proper RFC for the nim-lang/RFCs repo?

yes

@timotheecour
Copy link
Member Author

nimble does not support "system-installed" dependencies
If we want to employ nimble path for fusion, we would still need support within nimble itself

wondering if new nimbledeps fits the bill:

Nimble now supports project local dependency mode - if a nimbledeps directory exists within a project, Nimble will use it to store all package dependencies instead of ~/.nimble/bin. This enables isolation of a project and its dependencies from other projects being developed.

@alaviss
Copy link
Contributor

alaviss commented Oct 17, 2020

It doesn't. nimbledeps functions like: [[ -d nimbledeps ]] && export NIMBLE_DIR=$PWD/nimbledeps from what I can tell.

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 a pull request may close this issue.

2 participants