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

all imported external projects (tinycc, linenoise, etc) should have their own git history (via submodule or other) #206

Closed
4 of 6 tasks
timotheecour opened this issue Apr 2, 2020 · 7 comments
Labels
wontfix This will not be worked on

Comments

@timotheecour
Copy link
Member

timotheecour commented Apr 2, 2020

right now nim imports several projects by copying their files directly into nim's own git index. This makes it hard/impossible to update nim's version of those from upstream, and is just bad general practice anyways, for so many reaons. eg, it's impossible to tell which changes are our own modifications to those projects vs which ones are from upstream.
Eg: it's impossible to review a PR like nim-lang/Nim#6593 (for tinyc) or nim-lang/Nim#5357 (for linenoise) as we can't tell which change is form upstream vs local.

There is a much simpler, standard, way:

  • either using git submodules for these (which is essentially a git hash that gets version controlled)
  • or some logic to git clone those external projects at a specific git hash (very similar), to be called inside koch boot or build_all.{sh/bat}

main benefit: allows our versions of these external projects to be rebased against upstream regularly, while being able to maintain our own modifications of this (and independenly of nim's own git repo). We'd maintain our own forks if any modification to upstream is needed.

example external projects that should be moved out of Nim's index

  • Nim/tests/deps/jester-#head/
  • Nim/tests/deps/opengl-1.1.0/
  • Nim/tests/deps/x11-1.0/
  • Nim/tests/deps/zip-0.2.1/
@Araq
Copy link
Member

Araq commented Apr 2, 2020

Well tests are tests it doesn't matter they are not updated, we also test important_packages. I don't understand the Nim/csources point.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 2, 2020

Well tests are tests it doesn't matter they are not updated

fine, ok to ignore Nim/tests/ from this RFC

I don't understand the Nim/csources point.

until recently, csources kept being updated at each new major nim release:
... v0.15.2 v0.16.0 v0.17.0 v0.17.2 v0.18.0 v0.19.0 v0.20.0
which made bootstrapping easier because we didn't have to keep maintaining old features (preventing simplifications in the language) and could benefit from new features.

but it stopped at v0.20.0 and now csources is frozen, even though there was 1.0 thru 1.0.6 being released in the meantime.
Whether we need a csources2 (with the benefit of being lighter weight although that's moot with git clone --depth 1) or we need to unfreeze csources so that the bootstrapping nim can be kept not too old.

I'd be happy enough if that update to csources (or csources2) could come right after v1.2 (assuming that's coming in weeks, not months?).

In which case we can add either csources2/csources as a git submodule or add a reference to the git hash/tag in nim's repo (basically equivalent), say in Nim/deps.json that's checked in (thus satisfying nim-lang/Nim#11457) and update build_all.{sh,bat} to checkout at the corresponding revision instead of HEAD. Quite easy IMO.

note:

if not using submodules (eg nim-lang/Nim#2681), we'll use Nim/deps.json which would for now contain:

{
"nimble": {
  "url": "https://github.com/nim-lang/nimble",
  "hash": "4007b2a778429a978e12307bf13a038029b4c4d9",
},
"z3": {
  "url": "https://github.com/Z3Prover/z3",
  "hash": "65de3f748a6812eecd7db7c478d5fc54424d368b",
},
"csources": {
  "url": "https://github.com/nim-lang/csources",
  "hash": "...",
},
"linenoise": ...,
"tinyC": ...,
}

urls are needed so that we can update them as needed with private forks in case some modifications are needed, and this would all be transparent for scripts.

  • build_all.sh would read Nim/deps.json
  • ditto for koch (which hardcodes NimbleStableCommit, Z3StableCommit currently)

@Araq
Copy link
Member

Araq commented Apr 2, 2020

Well yeah, we hardcode it in koch.nim, what's the benefit of hardcoding it somewhere else? And yeah, we don't use git submodules.

And why is TinyC still even part of Nim? Let's remove it altogether.

@timotheecour
Copy link
Member Author

Well yeah, we hardcode it in koch.nim, what's the benefit of hardcoding it somewhere else

correct for other dependencies except for csources, which is needed before koch is even built.

@Araq
Copy link
Member

Araq commented Apr 3, 2020

csources is frozen though and even though you may disagree this works very well.

@Araq
Copy link
Member

Araq commented Oct 28, 2020

The way we currently do this is fine, let's agree to disagree.

@Araq Araq closed this as completed Oct 28, 2020
@timotheecour
Copy link
Member Author

timotheecour commented Oct 28, 2020

At least linenoise should still be cloned (possibly from a fork with changes for nim integration) otherwise updating from upstream is a major pain. This should be done like was done here nim-lang/Nim#13850 that did the same for tinyc. And possibly other as mentioned in OP.

Marking as wontfix until then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants