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

(enh) wren_to_c_string processes all files at once #80

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joshgoebel
Copy link
Contributor

Fixes the inc builder to not care whether the wren
source files have a trailing line break or not.

Fixes the `inc` builder to not care whether the wren
source files have a trailing line break or not.
@ChayimFriedman2
Copy link

This is nice, but this tool is intended for internal usage, so I'm not sure we need this. Ask @ruby0x1 though.

@joshgoebel
Copy link
Contributor Author

joshgoebel commented Apr 27, 2021

It will bite anyone adding new functionality to the CLI (ie, CLI development in general) by dropping new .wren files into modules (which I hit trying to add networking)... I can't see a reason not to just go ahead and fix it. I'd call the current behavior broken.

I find the new code slightly more explicit and easier to follow as well without that conditional in the middle of the loop.

You could probably just do this all with list comprehension but I don't know Python that well.

@joshgoebel joshgoebel changed the title fix wren_to_string (allow files not ending in \n) (enh) wren_to_c_string processes all files at once Apr 28, 2021
@joshgoebel
Copy link
Contributor Author

@ruby0x1 You mentioned simplifying the utility to just process all the wren files at once. Done.

But is there any reason we need separate output files? Can we just consider these another artifact of the build process and consolidate them into one larger modules.wren.inc file? That also means less effort to add new modules in the future, no need to update makefiles, etc.

@clsource
Copy link

I think another option would be like DOME where the wren to c string is included in the CLI tool (and have a small C binary helper too). So there will be less dependency of python

$ dome -e | --embed sourceFile [moduleVariableName] [destinationFile]

@joshgoebel
Copy link
Contributor Author

joshgoebel commented Apr 28, 2021

I already rewrote it in Wren but the worry was that it's technically part of the compile pipeline... so now you can't compile Wren unless you've already compiled Wren, etc. - even though that isn't strictly true (since the inc files are vendored). I'd love to replace all the Python scripts with Wren scripts honestly. :-)

I don't think it has to be "built into the CLI" but it could just be tools/build_c_inc.wren, etc. Just another helpful Wren script. :)

If your'e curious: https://github.com/joshgoebel/wren-cli/tree/dogfood

@clsource
Copy link

Indeed. A small binary would be needed to interpret such build scripts.

Dome solved by using that https://github.com/domeengine/dome/blob/main/src/util/embed.c
And Wren lang have this small test.c https://github.com/wren-lang/wren/blob/main/test/main.c to execute tests. So maybe that executable can be recycled for writing the utils in Wren instead of Python :)

@joshgoebel
Copy link
Contributor Author

Well it's more about building it across all platforms, distributing it, etc... if we already had all that infrastructure in place I'm not sure why we just wouldn't ship a binary CLI - I don't think we need "something smaller". Our CLI is already tiny to be honest.

@clsource
Copy link

Using wren_cli for building wren_cli?
thats something interesting and could free wren_cli from python dependency

@joshgoebel
Copy link
Contributor Author

joshgoebel commented Apr 29, 2021

Yes. But it creates a circular dependency, that's the concern at least.

@clsource
Copy link

I though that was just including the wren 0.3 release executables and use them to build newer versions of wren. Since the build scripts do not use lots of apis an older already compiled version would be enough 👍

@clsource
Copy link

clsource commented May 1, 2021

There is an alternative way of building the cli. That is including a copy of wren.inc files that are not supposed to be modified. Only for compiling the tool with the modules needed for the scripts.

imagen

So first you can compile a wren_cli with the cached incs. Then using that you process the utils scripts made in wren. Finally you can compile the final wren_cli with the latest wren incs.

@joshgoebel
Copy link
Contributor Author

Are the inc files not already included in the repo? I thought they were...

@clsource
Copy link

clsource commented May 2, 2021

Yes but they can be easily be modified for testing the new versions. So is better to have a non modified ones in a cache that is not supposed to be touched. This can be either in a serie of files or in a big variable of C chars.

Using this cached versions can be behind a compilation flag like

// modules.c
#include <stdlib.h>
#include <string.h>

#include "modules.h"

#ifdef USE_WREN_CACHED_MODULES
  #include "io.wren.cache.inc"
  #include "os.wren.cache.inc"
  #include "repl.wren.cache.inc"
  #include "scheduler.wren.cache.inc"
  #include "timer.wren.cache.inc"
#else
  #include "io.wren.inc"
  #include "os.wren.inc"
  #include "repl.wren.inc"
  #include "scheduler.wren.inc"
  #include "timer.wren.inc"
#endif

@clsource
Copy link

clsource commented May 2, 2021

I think this is a conversation for another PR/Issue. This issue can be subdivided into 3 problems

1 - PR: Fix the newline bug
2 - PR: Fix circular dependency. Allow binary compilation for compiling Wren
3 - PR: Fix the python dependency. Allow Wren scripts to be executed for the compilaiton step. Resolving python issues like #97

@joshgoebel
Copy link
Contributor Author

I think this is a conversation for another PR/Issue

Agree.

Please open a new issue for 2/3 if you wish (I wouldn't jump straight to a PR - unless you're OK if it gets rejected). I feel like I've already received a little pushback on the idea... but perhaps if you write up a clear issue and make the case better than I did you will have better luck. :-)

@ruby0x1
Copy link
Member

ruby0x1 commented May 2, 2021

It's not solving any particular problem that has impact while working on the cli, and is unnecessary complexity at this time. (not referring the to new line fix).

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