Split source / output #13

WolfieWerewolf · 2019-02-16T18:27:07Z

Hi guys.. I am very impressed with the work you have done here; I wish I had known about it sooner. Once I got up and running (thanks to @zicklag) I thought I might be able to help out by contributing a few small changes.

Moved the hard coded webidl.Options to webidl.json in the project folder.
Added an optional field to webidl.Options for 'out' so we can specify the output folder.
Added an OUT field to the Makefile (ideall we want this all in one place but since we are using make I thought I would just follow the existing approach)
Modified Generate.hx to use the opts.out field to put the generated output into a target folder.. in my example it's ./webRoot.
Added a make clean to remove the generated output from webRoot

It should be possible to simply execute "make js" and have everything dump into webRoot. If you run something like live-server or the python simple server in there you should get the running wasm.

I have tested this on Linux Mint which seems entirely compatible with Ubuntu based on the tested / help that @zicklag provided. I don't have access to OSX / MacOS or Windows unfortunately.

If you are happy with this great, if there are changes you would like, I am happy to make them, if you don't like any of this then that's ok too ;-) I just wanted to see if I could help out.
Thanks for your consideration.
Kind Regards
Wolfie

zicklag · 2019-02-16T19:01:28Z

I haven't tested running it, but I think its a good idea. 👍

My only thought from looking at it is that we could maybe merge the splitout sample with the other one, or at least put them both in a samples folder.

WolfieWerewolf · 2019-02-16T19:06:35Z

@zicklag Excellent.. I would be more than happy to merge it into the existing sample.. I just didn't want to go ahead and do that unless I had some kind of indication from you guys that it would be acceptable to do so. I'll manually make the changes and let you know when that's done.. thanks.

zicklag · 2019-02-16T19:08:03Z

Also you might want to wait to hear from @ncannasse. Its his repo, I've just contributed. 😃

WolfieWerewolf · 2019-02-16T19:08:40Z

@zicklag ... cool, @ncannasse with your permission I would like to do as @zicklag suggested :-)

WolfieWerewolf · 2019-02-16T19:31:11Z

@zicklag ... while I have you here maybe you can answer something for me? What is the plan with regards to handling bindings for multiple c++ classes in the same project. The idl file in the sample is called point.idl presumably because the only thing it handles presently is the Point class but if I wanted to add a class (different header and implementation file) would I just append it to the same .idl file? And if so I suspect I will have to handle it somehow in the makefile? Perhaps make is a bit light for a larger workload if that's the case? Perhaps the work presently being done by make should be handled by a webidl executable? I'm not sure what the long term goals are for this project but it looks like something that I would like to make more industrial use of than just a few samples...

WolfieWerewolf · 2019-02-16T22:21:52Z

@zicklag well that was easier than I expected. I've gone ahead and updated my fork with the changes necessary to compile multiple c++ targets and generate the bindings. For now I updated the splitout example.

Handling parsing of #includes (Generalte line 92.. Haxe is refreshingly Functional these days.. )in generator so it is no longer necessary to supply them manually in the webidl.json file. I haven't tested cases where there might be duplicate includes so I have a note to test that and deduplicate the array if that happens (I suspect it will).
Added a Array field for sourceFiles in webidl.json and removed the hard coded source files in the sample project

From a user perspective:

All you have to do is ensure the sourceFiles in webidl.json are supplied in the array; it is no longer necessary to call out the includes specifically, it should parse / generate them dynamically now.
Update the Makefile supplying the cpp files on the libpoint.hdll and (libpoint_win lines if on Windows)

The makefile isn't terrible but I think it could still become troublesome having to make changes in two places.. I'll put some more thought into that.
If you guys could test that and let me know what you think I would appreciate it.
After you run make.js and launch a webserver in ./webRoot you should see the following output on the js console in your browser (tested on Chrome latest).

splitout/Splitout.hx:17: Result = 11,13 len=17.029386365926403
libpoint.js:8 This is Context!
libpoint.js:8 This is test!

zicklag · 2019-02-16T22:28:22Z

Sounds cool! I'm a little busy, but I'll let you know if I try it.

ncannasse · 2019-02-17T13:27:46Z

Hey there ! I'm glad there's interest in this project. ATM I have already many (=too much) repositories to monitor so i'm not sure I can invest a lot of time on webidl.

So if @zicklag wants to be added as a contributor and keep developing it, I will gladly give the corresponding rights :) I'm planning to use webidl for bullet HL support, as long as it doesn't break things here it's fine for me.

WolfieWerewolf · 2019-02-17T14:03:28Z

@ncannasse .. Hi Nick.. very good news, congratulations @zicklag you have been promoted!. I'll go ahead and make the changes you suggested. Thank you both.

zicklag · 2019-02-17T20:00:58Z

@ncannasse That would be great, thanks!

WolfieWerewolf · 2019-02-18T18:31:25Z

Hi @zicklag .. as per our discussion I have gone ahead and merged the samples as follows:

Created a top level samples folder in the project root
Renamed the existing sample to simple
The simple sample is an aggregation of the original and the changes described above in this PR.

I've additionally taken the liberty of changing the naming a little based on our discussions:

The Module name reflects the folderName+Module.hx --> SimpleModule.hx
The Entry point reflects the folder name as Upper Case -> Simple.hx
The IDL file reflects the folderName since the idl file is more related to the module than it's contents. -> simple.idl

When generating output the following naming is used:

We prefix the folder name with lib_ (snake case) following the examples by the emscripten guys since this is the generated js.
The custom js bindings are held in folder.js -> simple.js

I guess you are the new maintainer? If so could you please test at your convenience and let me know if any changes are needed or not. If not I am happy for you to merge this as it is. I will have further enhancements to contribute later this week.

@ncannasse .. Nick, you said something important above "so long as we don't break anything".. can you be specific about what that means? What can I test against to ensure that nothing is broken and / or if there are breaking changes what can I use to document against to keep you current?

Also I plan to update the readme to reflect these changes but since it includes a URL which is not yet merged to master I haven't pushed that yet.

Thanks
Kind Regards
/W

zicklag · 2019-02-18T19:12:24Z

@WolfieWerewolf I'll check it out as soon as I can. 🙂

As far as what we can't break, it's bullet bindings for HashLink.

Edit: Also about the Readme URL, I think you can just put that in there, and when the PR is merged it will be valid right?

WolfieWerewolf · 2019-02-18T19:18:42Z

@zicklag ... excellent thanks.. I'll test against that shortly. There will likely need to be some changes to those bindings to make use of this PR.. if so I'll create a related PR for that project.
Re the URL... I don't think so.. the url is hard coded in the MD and will point to my fork, not the active master... I mean I could point it to what it would be once merged but I'll have to test that anyway..

WolfieWerewolf · 2019-02-18T20:40:42Z

@zicklag .. mate.. I haven't tried to compile this yet but it looks to me like the Haxe Bullet bindings include a fork of the webidl that it was using at the time and doesn't reference this project?
https://github.com/armory3d/haxebullet/tree/master/Sources/webidl
And the webidl files look to be about 3 months old. I would suspect that various changes, not just the ones in this PR, could have had an impact on it. Has anyone tested this recently?

zicklag · 2019-02-18T20:44:19Z

That's not the repo I was talking about here. The one I'm talking about is the one that HashLink itself comes with: https://github.com/HaxeFoundation/hashlink/tree/master/libs/bullet.

BTW I've got a PR for Armory3D's Haxebullet to update it. That is part of the Kha integration I mentioned in the other issue. ;) armory3d/haxebullet#27

WolfieWerewolf · 2019-02-18T20:48:58Z

oops.. my bad. I'm sorry I'm not following there's a lot going on. Do you still want me to verify against the bullet stuff in hashlink (using the correct URL this time)... sorry for my confusion.. I am just getting caught up with all the dependencies etc..

zicklag · 2019-02-18T20:55:35Z

No problem. You could test it if you want. It should still work fine because I think it pretty much just uses the generated bullet.cpp file from WebIDL and our changes have been mostly to the JavaScript generation and the export locations, which aren't used by that repo directly anyway. Still could be good to test. I think the bullet.cpp file should actually end up identical to the one already there if you generate bindings with the included WebIDL file.

WolfieWerewolf · 2019-02-18T21:03:17Z

Ok good info thanks.. I will test just to be sure.. I did make some changes to the way the configuration is handled though.. I moved the static config out of the Module to a json file so that might need some tweaking.. thanks for the info

zicklag

OK here's my review. If you don't agree with a review just tell me, I'm not completely set on all of the comments. 🙂

BTW it did work. 😉

zicklag · 2019-02-19T23:08:29Z

samples/simple/SimpleModule.hx

+
+class SimpleModule {
+
+	static var json = {


I'm thinking we can simplify this block like so:

Suggested change

static var json = {

static var json = haxe.Json.parse(sys.io.File.getContent("./webidl.json"));

The GitHub auto-commit thing won't remove the 4 lines below, so we'll need to do that manually.

zicklag · 2019-02-19T23:09:57Z

.vscode/c_cpp_properties.json

@@ -0,0 +1,16 @@
+{


I'm assuming this file wasn't meant to be in here as you added .vscode to the .gitignore?

Correct.. I should not have pushed that.

zicklag · 2019-02-19T23:12:40Z

samples/simple/index.html

@@ -0,0 +1,9 @@
+<!doctype html>


Do we need this index.html and the one in webRoot?

Well I like to not edit anything in webRoot directly and treat it all as generated.. You'll notice that the new build copies the index.html from the source root (which is still the project root) every time you compile.. so changes should be made in sources not in webRoot (output).

samples/simple/webidl.json

zicklag · 2019-02-19T23:30:27Z

webidl/Generate.hx

 			add("");
-			add(StringTools.trim(opts.includeCode));
+			for(l in sys.io.File.getBytes(if(sys.FileSystem.exists(opts.out+ "/" + sFile)){


I find this block of code very difficult to read. This is a bit cleaner:

var path = null; for(sFile in opts.sourceFiles){ add(""); path = FileSystem.exists(opts.out + "/" + sFile) ? opts.out + "/" + sFile : sFile; for(l in sys.io.File.getContent(path).split("\n")){ if(StringTools.startsWith(l, "#include")){ add(StringTools.trim(l)); } } }

Also for large projects would this be a performance concern, going through all of the source files that could represent a large library? I don't know how long that takes, it probably isn't a problem, but I thought I'd bring it up. Maybe we should test it with a bunch of files or add a autoIncludeIncludes ( 😉 ) flag or something like that to the webidl.json ( maybe with a better name like detectIncludes ).

Looks good to me.. I'm coming from Kotlin and I've been embracing it's functional side so I've become used to long functions.. however you want it man is fine with me.

zicklag · 2019-02-19T23:34:23Z

webidl/Generate.hx

@@ -399,7 +399,7 @@ template<typename T> pref<T> *_alloc_const( const T *value ) {
 			if( p.substr(0, 2) == "-O" )
 				hasOpt = true;
 		if( !hasOpt )
-			params.push("-O2");


I think O2 is probably a good default. I'm not really set on it if you have a good reason, but I think O2 made sense.

And that is also an oversight on my part (good catch).. I dropped down to -O0 so I could more easily read the output.. you are correct this should be optimised.

zicklag · 2019-02-19T23:52:35Z

webidl/Generate.hx

@@ -412,26 +412,32 @@ template<typename T> pref<T> *_alloc_const( const T *value ) {
 		var outFiles = [];
 		sources.push(lib+".cpp");
 		for( cfile in sources ) {
-			var out = cfile.substr(0, -4) + ".bc";
-			var args = params.concat(["-c", cfile, "-o", out]);
+			var sourcePath = if(sys.FileSystem.exists(opts.out+ "/" + cfile)){


I might be missing something, but why are we looking in the output directory as a possible location for a source file?

Because it is the only place where the lib_project.cpp is going to ever exist; it's generated.

That makes sense, in that case I would just update the if statement to be single lined:

var sourcePath = sys.FileSystem.exists(opts.out + "/" + cfile) ? opts.out + "/" + cfile : cfile;

zicklag · 2019-02-19T23:55:46Z

webidl/Generate.hx

 		]);
 		var output = "SOURCES = " + outFiles.join(" ") + "\n";
 		output += "all:\n";
 		output += "\t"+emcc+" $(SOURCES) " + args.join(" ");
 		sys.io.File.saveContent(tmp, output);
 		command("make", ["-f", tmp]);
 		sys.FileSystem.deleteFile(tmp);
+		sys.io.File.copy("index.html", opts.out+"/index.html");


I see that this explains the two index.html files. If we have the favicon already in the webRoot folder when it builds I don't see any reason that we can't have the index.html file in there as well and leave out this copy.

Sure.. I don't mind either way..

In order to preserve the sources separate from the output dir concept, which I think is good, we could create like a "bundled" directory maybe and add that to the webidl.json options? Every file or folder in like a bundledResources option would be copied right into the output directory.

Edit: BTW I put this comment on the wrong review so I moved it and hid the old one.

Co-Authored-By: WolfieWerewolf <wolfiewerewolf@gmail.com>

WolfieWerewolf · 2019-02-20T00:14:46Z

Mate.. that was an excellent code review.. thank you..

zicklag · 2019-02-20T00:21:35Z

No problem. That was actually the first time I've ever reviewed a PR to a repo that I maintain.

WolfieWerewolf · 2019-02-20T00:46:22Z

Nice.. I do appreciate it because no matter how much I look at code I'll always miss something.. like the .vscode thing... I'll go through this again with our comments, make the necessary changes and push that up.. likely tomorrow mate.. thanks again.

WolfieWerewolf added 2 commits February 16, 2019 14:17

Implemented a new sample as a proof of concept source / output split.

8819ef1

Merge branch 'master' of https://github.com/ncannasse/webidl

e890be1

zicklag mentioned this pull request Feb 16, 2019

[Question] What is the plan with regards to handling bindings for multiple c++ classes in the same project #14

Open

WolfieWerewolf added 2 commits February 16, 2019 17:32

Consolidating the various options that are sent to the Generator.

e758a27

Compiling Multiple c++ targets

f364242

small fixes

60d390b

WolfieWerewolf added 2 commits February 18, 2019 14:20

Merged samples

ffdd9d8

more

6a3a504

zicklag requested changes Feb 19, 2019

View reviewed changes

Update samples/simple/SimpleModule.hx

8c4c91a

Co-Authored-By: WolfieWerewolf <wolfiewerewolf@gmail.com>

	static var json = {
	static var json = haxe.Json.parse(sys.io.File.getContent("./webidl.json"));

Split source / output #13

Are you sure you want to change the base?

Split source / output #13

Conversation

WolfieWerewolf commented Feb 16, 2019

zicklag commented Feb 16, 2019

WolfieWerewolf commented Feb 16, 2019

zicklag commented Feb 16, 2019

WolfieWerewolf commented Feb 16, 2019

WolfieWerewolf commented Feb 16, 2019

WolfieWerewolf commented Feb 16, 2019

zicklag commented Feb 16, 2019

ncannasse commented Feb 17, 2019

WolfieWerewolf commented Feb 17, 2019

zicklag commented Feb 17, 2019

WolfieWerewolf commented Feb 18, 2019 • edited Loading

zicklag commented Feb 18, 2019 • edited Loading

WolfieWerewolf commented Feb 18, 2019 • edited Loading

WolfieWerewolf commented Feb 18, 2019

zicklag commented Feb 18, 2019

WolfieWerewolf commented Feb 18, 2019

zicklag commented Feb 18, 2019

WolfieWerewolf commented Feb 18, 2019

zicklag left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WolfieWerewolf Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zicklag Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

WolfieWerewolf commented Feb 20, 2019

zicklag commented Feb 20, 2019

WolfieWerewolf commented Feb 20, 2019

WolfieWerewolf commented Feb 18, 2019 •

edited

Loading

zicklag commented Feb 18, 2019 •

edited

Loading

WolfieWerewolf commented Feb 18, 2019 •

edited

Loading

zicklag left a comment •

edited

Loading

WolfieWerewolf Feb 20, 2019 •

edited

Loading

zicklag Feb 20, 2019 •

edited

Loading