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

[superseded] niminst: make linenoise.h optional #13413

Conversation

timotheecour
Copy link
Member

refs: 2b368bc#commitcomment-37273026
/cc @drjdn does this work for you?

timotheecour referenced this pull request Feb 14, 2020
* fix nightlies linenoise regression

* fix other installers
tools/niminst/niminst.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member Author

timotheecour commented Feb 14, 2020

adding a flag to niminst (eg --bundleNim) and then passing it from ./koch csources ended up causing a rabbit hole of complexity/extra code just for that 1 boolean flag. Instead I've changed the code so that this works:

./koch csource -d:release -d:nimBundleNimExtra

and made a reference to that in ./koch --help

@drjdn
Copy link

drjdn commented Feb 14, 2020

@timotheecour I can confirm that this now works again as expected.

@Araq
Copy link
Member

Araq commented Feb 17, 2020

With this PR nightlies will be broken once again. Can we please revert this linenoise patch and make nim secret its own tool. It's also blocking my merge of #13065 !!

@drjdn
Copy link

drjdn commented Feb 23, 2020

@timotheecour could you please back out your modifications to niminst. Your last changes fix my initial issue but I'm onside with @Araq that this should be a separate tool. Currently we have the following in:

"wrappers", "wrappers/linenoise",

which basically means if you want to use nim's compiler internals and export that to standalone C code you need to copy around a header for a line editing library that is very unlikely to be needed. Exporting C code like this is one of nim's killer features in my opinion. Until an actual REPL that handles the whole language and FFI exists (which would be awesome) I don't see why this needs to be in the compiler directory and definitely not in niminst.

@timotheecour
Copy link
Member Author

all right, see #13478 ; if it gets merged we can close this

@narimiran
Copy link
Member

if it gets merged we can close this

Since it has merge_when_passes_CI label, I'm closing this one already.

@narimiran narimiran closed this Feb 24, 2020
@timotheecour timotheecour deleted the pr_fix_linenoise_niminst_optional branch February 24, 2020 10:53
@timotheecour timotheecour changed the title niminst: make linenoise.h optional [superseded] niminst: make linenoise.h optional Feb 24, 2020
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 this pull request may close these issues.

4 participants