-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
added Atlas helper tool #18497
added Atlas helper tool #18497
Conversation
I disagree that this needs or should be in the core repo. Justification for it to not be in the core repo: Nimble (and other PMs) won't need to depend on latest repo to use Why not just create an atlas repo and allow PMs to use it as a submodule? |
Well ok, but can Nimble itself depend on Nimble packages? I suppose the CI could setup some "git clone" command, but still. |
atlas/atlas.nim
Outdated
|
||
proc isCleanGit(dir: string): string = | ||
result = "" | ||
let (outp, status) = osproc.execCmdEx("git diff") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe can reuse/expand https://nim-lang.github.io/Nim/gitutils.html ?. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't do library development for a reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for what reason though? code reuse (especially within same git repo, where you have full control over breaking changes, std/private/gitutils is still private so can be improved as needed) is better than duplicating the logic, and duplicating bugfixes;
koch also has ensureCleanGit
; it its case you could argue bootstrap should minimize dependencies, but for any tool used after bootstrap, code reuse makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's one line of code, if I got it wrong, I'll reconsider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gitutils.retryCall seems pretty good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about I merge this and you create more PRs improving it? I definitely also have other things to do...
atlas/atlas.nim
Outdated
projectDir, workspace: string | ||
hasPackageList: bool | ||
keepCommits: bool | ||
p: Table[string, string] # name -> url mapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe URL can be uri.Uri
?, more future proof,
so we do not end up like HttpClient
with url: string or Uri
everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be done later.
It can in theory, in practice we haven't settled on how to do it. But submodules can work well |
If the upcoming Nimble release cannot depend on Nim 1.6 for some reason, you can always depend on Nim's repo as a submodule. I forgot the most important point: The Nimble file parser uses the Nim compiler as an API and in the past outsourcing these things into separate repositories didn't work at all: Nimsuggest moved back into the Nim repo, c2nim ended up copying the parts of the Nim compiler that it needs and Nimble ended up calling the Nim compiler as a process. |
This is the idea behind https://github.com/disruptek/ups -- I'm trying to move all the PM logic into a small library so we can standardize and debug it. Nimph already uses this thing. But if you'd rather the compiler was also a package manager, sure... |
👍 Works for me. I like the fact that it uses git to get the packages.json file instead of downloading it with httpclient. Ship it. |
atlas/parse_requires.nim
Outdated
yield tok | ||
|
||
when isMainModule: | ||
for x in tokenizeRequires("jester@#head >= 1.5 & <= 1.8"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be turned into a real test (ideally seperate module, that's what import {.all.} is for when needed) that testament can test, like it does for other tools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct but we don't have any tests yet.
So we still have the dependency on the Nim compiler sources, that's pretty bad. I think minimising the Nimble file format should include its own parser that doesn't require the bulk of the Nim parser. |
Well I won't write a new parser. Instead Nimble can ask the Atlas tool to parse the file for its needs, much like it does ask the Nim compiler today. |
* added Atlas helper tool * further improvements
Justification for adding it to the core repository: PMs (Nimble included) should be able to import
parse_requires.nim
for a common ground.The "atlas" tool is something that I frequently need and could eventually offer a Nim "bundling" process. Comparable to Ruby does its stdlib evolution via Gemification