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

Change nim's nimble files to make it installable #20179

Merged
merged 2 commits into from
Aug 31, 2022

Conversation

yyoncho
Copy link
Contributor

@yyoncho yyoncho commented Aug 8, 2022

I went for this minimal solution because it seems like compiler.nimble and
nimsuggest.nimble are not in use

@yyoncho
Copy link
Contributor Author

yyoncho commented Aug 9, 2022 via email

@dom96
Copy link
Contributor

dom96 commented Aug 9, 2022

We can just set up an alias: https://github.com/nim-lang/packages#renaming-packages (so that compiler points to the new nim package).

@yyoncho
Copy link
Contributor Author

yyoncho commented Aug 9, 2022

We can just set up an alias: https://github.com/nim-lang/packages#renaming-packages (so that compiler points to the new nim package).

That is what I am working on.

@yyoncho
Copy link
Contributor Author

yyoncho commented Aug 9, 2022

We can just set up an alias: https://github.com/nim-lang/packages#renaming-packages (so that compiler points to the new nim package).

That is what I am working on.

Actually, I started working on the opposite - add alias nim -> compiler. But having it the way you suggested will make the code simpler and easier to understand on nimble side.

@dom96
Copy link
Contributor

dom96 commented Aug 9, 2022

You shouldn't need any changes in Nimble for this afaik. Aliases are already supported.

@yyoncho
Copy link
Contributor Author

yyoncho commented Aug 9, 2022

You shouldn't need any changes in Nimble for this afaik. Aliases are already supported.

Yeah. But I had to redo my nimble PR if we add an alias nim -> compiler because it threats "nim" in a special way. But following your suggestion we should be good to go if renaming compiler compiler package to nim + adding alias is fine for everyone.

@yyoncho
Copy link
Contributor Author

yyoncho commented Aug 9, 2022

Here is the corresponding change in packages repo: nim-lang/packages#2317

yyoncho added a commit to yyoncho/packages that referenced this pull request Aug 11, 2022
@arnetheduck
Copy link
Contributor

is the "compiler" only the compiler or the compiler and the standard library? if it's the former, "compiler" should be the main package name, if it's the latter, "nim" - in the future, it's quite possible that one would want to have a "nim" package that contains the compiler and std lib as dependencies (specially if the std lib is split up into multiple nimble packages)

@yyoncho
Copy link
Contributor Author

yyoncho commented Aug 11, 2022

@arnetheduck compiler ATM has only the sources. I am adding the binaries as well + pretty much the content of the nimlang tar delivery.

@dom96
Copy link
Contributor

dom96 commented Aug 11, 2022

Hm, that's a good point. The current "compiler" package is just the compiler sources (and nimsuggest too for some reason?)

The compiler itself also depends on the stdlib though so if we do have a separate stdlib package the compiler will also need to depend on it.

@arnetheduck
Copy link
Contributor

and nimsuggest too for some reason?

presumably because it lives in the same directory? I imagine that one would want a separate nimble package for every (significant) tool. This would require some reorg of the repo, because afaik, nimble requires that each "project" is in its own folder, right?

I guess this is tied to the fact that nimsuggest was developed as a when jungle rather than using a clear and well-defined compiler API - presumably, with the compiler package, one would treat the compiler as any other library, ie with a stable API etc, then nimsuggest would call that API - that would likely require some additional refactoring though, to untangle suggest and other tools from nim-the-compiler itself.

@yyoncho
Copy link
Contributor Author

yyoncho commented Aug 15, 2022

presumably because it lives in the same directory?

It is due to this line: https://github.com/nim-lang/Nim/blob/devel/compiler.nimble#L7 There is also nimsuggest.nimble so I suspect that the idea was to have it like a subpackage.

@yyoncho
Copy link
Contributor Author

yyoncho commented Aug 15, 2022

Hey @Araq , can you take a look? Based on our previous conversion I believe that you don't want to structure the repo in a way to make it "nimble compatible" so splitting nim repo into separate components (e. g. stdlib, compiler, nimsuggest, etc) is no go.

If that is the case, then it seems to me if we want to have separate nimble packages(it is not clear to me whether there is such a need) then at some point we could implement support for multiple packages in a single repo.

@dom96
Copy link
Contributor

dom96 commented Aug 15, 2022

If that is the case, then it seems to me if we want to have separate nimble packages(it is not clear to me whether there is such a need) then at some point we could implement support for multiple packages in a single repo.

We already support that. See https://github.com/nim-lang/nimble#package-urls

@Araq Araq added the merge_when_passes_CI mergeable once green label Aug 24, 2022
- needs nim-lang#20168 to make the stuff working

I went for this minimal solution because it seems like `compiler.nimble` and
`nimsuggest.nimble` are not in use
Araq pushed a commit to nim-lang/packages that referenced this pull request Aug 24, 2022
* Rename compiler to nim

- see nim-lang/nimble#1017 and nim-lang/Nim#20179

* Better description of the Nim package

Co-authored-by: zah <zahary@gmail.com>
@Varriount Varriount requested a review from dom96 August 31, 2022 19:05
@Varriount Varriount added the Requires Dom96 To Merge PR should only be merged by Dom96 label Aug 31, 2022
@Varriount Varriount merged commit fb27734 into nim-lang:devel Aug 31, 2022
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from fb27734

Hint: mm: orc; threads: on; opt: speed; options: -d:release
163907 lines; 16.961s; 840.906MiB peakmem

@yyoncho
Copy link
Contributor Author

yyoncho commented Feb 6, 2023

Hey @narimiran , can you backport this and #21314 ?

yyoncho added a commit to yyoncho/Nim that referenced this pull request Feb 7, 2023
- needs nim-lang#20168 to make the stuff working

I went for this minimal solution because it seems like `compiler.nimble` and
`nimsuggest.nimble` are not in use

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>
narimiran pushed a commit that referenced this pull request Feb 9, 2023
- needs #20168 to make the stuff working

I went for this minimal solution because it seems like `compiler.nimble` and
`nimsuggest.nimble` are not in use

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>
(cherry picked from commit fb27734)
@narimiran
Copy link
Member

Hey @narimiran , can you backport this and #21314 ?

Both are now backported.

@ringabout
Copy link
Member

@yyoncho Hi, this PR seems to break the version-1-6 CI. Could you have a look at when you have some free time?

2023-02-09T05:09:06.4342746Z Test failed: /home/vsts/work/1/s/nimsuggest/tests/tchk1.nim
2023-02-09T05:09:06.4343275Z   Expected:
2023-02-09T05:09:06.4344500Z chk	skUnknown		Hint	???	0	-1	">> (toplevel): import(dirty): tests/tchk1.nim [Processing]"	0
2023-02-09T05:09:06.4345540Z chk	skUnknown		Error	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	12	0	"identifier expected, but got \'keyword template\'"	0
2023-02-09T05:09:06.4346411Z chk	skUnknown		Error	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	14	0	"nestable statement requires indentation"	0
2023-02-09T05:09:06.4347411Z chk	skUnknown		Error	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	12	0	"implementation of \'foo\' expected"	0
2023-02-09T05:09:06.4348146Z chk	skUnknown		Error	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	17	0	"invalid indentation"	0
2023-02-09T05:09:06.4349138Z chk	skUnknown		Hint	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	12	9	"\'foo\' is declared but not used [XDeclaredButNotUsed]"	0
2023-02-09T05:09:06.4350217Z chk	skUnknown		Hint	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	14	5	"\'main\' is declared but not used [XDeclaredButNotUsed]"	0
2023-02-09T05:09:06.4350707Z 
2023-02-09T05:09:06.4351112Z   But got:
2023-02-09T05:09:06.4351903Z chk	skUnknown		Hint	???	0	-1	">> (toplevel): import(dirty): nimsuggest/tests/tchk1.nim [Processing]"	0
2023-02-09T05:09:06.4352918Z chk	skUnknown		Error	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	12	0	"identifier expected, but got \'keyword template\'"	0
2023-02-09T05:09:06.4353723Z chk	skUnknown		Error	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	14	0	"nestable statement requires indentation"	0
2023-02-09T05:09:06.4354699Z chk	skUnknown		Error	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	12	0	"implementation of \'foo\' expected"	0
2023-02-09T05:09:06.4355425Z chk	skUnknown		Error	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	17	0	"invalid indentation"	0
2023-02-09T05:09:06.4356707Z chk	skUnknown		Hint	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	12	9	"\'foo\' is declared but not used [XDeclaredButNotUsed]"	0
2023-02-09T05:09:06.4357775Z chk	skUnknown		Hint	/home/vsts/work/1/s/nimsuggest/tests/tchk1.nim	14	5	"\'main\' is declared but not used [XDeclaredButNotUsed]"	0

ref https://dev.azure.com/nim-lang/Nim/_build/results?buildId=25957&view=logs&jobId=c6054849-8341-5e23-b888-79fd7ec95d3a&j=ba7bbfa4-f55c-5c34-6d52-1b6b4edd3f37&t=01b6eca1-ed82-5532-a6ed-8d9a0cd0188a

@yyoncho
Copy link
Contributor Author

yyoncho commented Mar 7, 2023

@ringabout I am back from a vacation, I will take a look if it is still relevant.

@ringabout
Copy link
Member

Nope, it has been fixed.

capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
- needs nim-lang#20168 to make the stuff working

I went for this minimal solution because it seems like `compiler.nimble` and
`nimsuggest.nimble` are not in use

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green Requires Dom96 To Merge PR should only be merged by Dom96
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants