Feature/new Bindings generator #946

realvictorprm · 2019-08-11T18:07:19Z

Replace binder with a rewritten modern variant

The old binder does not work for the new specification. This PR aims to provide a completely new written version using ADL for xplatform native calls. The generator will be completely written in F# as it's best suited for this task.

Testing status

Tests will just verify the transformations and filtering done during the generation process

Comments

This has been decided after a long conversation with Fred, jvbl and Varon with me.

…ignature & will notify if a function references an unregistered enum group

NogginBops · 2019-08-11T22:43:53Z

Is writing the generator in F# really a good idea?

Almost the entire project is written in C# and it is expected that only C# developers would be interested in contributing to this project. That means that we cannot expect future contributors to have extensive knowledge in F#. So if we write the entire bindings in F# we are in a situation where future maintainers might not be able to use the generator anymore because there is no-one left who knows anything about F#.

I'm sorry to say but this does not feel like it is a good idea in the long-term (maybe even the short term as it will take longer to complete and fewer contributors can contribute). Especially looking a the history of this project (changing maintainers and contributors a lot) it feels like this is not the move we want to make.

Sorry to revive the discussion @realvictorprm @varon @jvbsl :)

varon · 2019-08-12T13:46:46Z

@realvictorprm - Looking good so far.

Can we avoid committing the gl4.xml ? Would be ideal if we could manage it as a dependency using the locked commit ID as part of the build process.

realvictorprm · 2019-08-12T14:05:15Z

Yes this is no Problem and planned. I haven’t done it yet. Gesendet von Mail für Windows 10 Von: varon Gesendet: 12 August 2019 15:46 An: opentk/opentk Cc: Victor Peter Rouven Müller; Mention Betreff: Re: [opentk/opentk] Feature/new generator (#946) @realvictorprm - Looking good so far. Can we avoid committing the gl4.xml ? Would be ideal if we could manage it as a dependency using the locked commit ID as part of the build process. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

…

--- This email has been checked for viruses by Avast antivirus software. https://www.avast.com/antivirus

deccer · 2019-08-12T19:42:00Z

build.fsx

+    |> Seq.concat
+    |> Seq.toList
+
+let testAssemblies = "tests/**/obj/Release/*Tests*.dll"


These should not be taken from the obj folder, but bin/Release

Thanks @deccer. We really appreciate ad-hoc reviews and such!

that makes much more sense lol, thanks a lot

build.fsx

deccer · 2019-08-12T19:44:19Z

build.fsx

+
+// Generate assembly info files with the right version & up-to-date information
+Target.create "AssemblyInfo" (fun _ ->
+    //TODO: Create and update the Directory.Build.props file with the version information taken from the releaseNotes value.


Maybe could be turned into an issue in a Project Build System? (Edit: I was referring to the TODO line)

deccer · 2019-08-12T19:46:09Z

build.fsx

+// This preserves subdirectories.
+Target.create "CopyBinaries" (fun _ ->
+    releaseProjects
+    |>  Seq.map (fun f -> ((System.IO.Path.GetDirectoryName f) @@ "bin/Release/netstandard2.0", "bin" @@ (System.IO.Path.GetFileNameWithoutExtension f)))


Would it make sense, to put that in a variable, for easier maintenance..... /netstandard2.0 i mean (Im not sure if the review thing here, keeps the thing i marked)

deccer · 2019-08-12T19:47:30Z

build.fsx

+
+let testAssemblies = "tests/**/obj/Release/*Tests*.dll"
+
+let nugetCommandRunnerPath = ".fake/build.fsx/packages/NuGet.CommandLine/tools/NuGet.exe" |> Fake.IO.Path.convertWindowsToCurrentPath


Maybe move the nuget.exe path into a variable on top of the script as well

I'll see later if I can remove it, to my memory I think I can remove it...

deccer · 2019-08-12T19:48:25Z

build.fsx

+    !! "tests/**/*.??proj"
+
+let allProjects =
+    [toolProjects ; releaseProjects; testProjects ]


me being picky here, run a Format Document over the files please :)

Will run Fantomas later

src/Generator/Program.fs

src/binding/Binding/Initializer.cs

src/binding/Binding/Types.cs

…right now, sorry!

…ild script accordingly

frederikja163 · 2019-08-17T08:46:20Z

Is writing the generator in F# really a good idea?

Almost the entire project is written in C# and it is expected that only C# developers would be interested in contributing to this project. That means that we cannot expect future contributors to have extensive knowledge in F#. So if we write the entire bindings in F# we are in a situation where future maintainers might not be able to use the generator anymore because there is no-one left who knows anything about F#.

I'm sorry to say but this does not feel like it is a good idea in the long-term (maybe even the short term as it will take longer to complete and fewer contributors can contribute). Especially looking a the history of this project (changing maintainers and contributors a lot) it feels like this is not the move we want to make.

Sorry to revive the discussion @realvictorprm @varon @jvbsl :)

Just for future reference, we gave the response over discord
https://discordapp.com/channels/337627185248468993/525453014618865664/610432160389791763

src/Generator/Program.fs

Kavignon · 2019-08-19T20:26:12Z

Let's keep the history clean! Whenever there's an in-progress investigation/implementation, it's ok to do as many commits as possible. But when done, squash them into one which sums up the work done to finish solving the problem :)

realvictorprm · 2019-08-19T20:42:19Z

Before I've used it but it could be that I've removed all usages of it. Kevin Avignon <notifications@github.com> schrieb am Mo., 19. Aug. 2019, 22:35:

…

***@***.**** commented on this pull request. ------------------------------ In src/Generator/Util.fs <#946 (comment)>: > @@ -0,0 +1,72 @@ +module Util + +open Types + +let inline (</>) path1 path2 = System.IO.Path.Combine(path1, path2) + +type MaybeBuilder() = Except for when you define the maybe builder, I haven't seen a use for it which triggered my question. Maybe there's something I'm missing?? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#946?email_source=notifications&email_token=AGB4Z4FQEI2PBLD2MATPKNLQFL7ZLA5CNFSM4IK4LXE2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCB72BEI#discussion_r315399469>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGB4Z4CURHCQ4JSE4O6X5XLQFL7ZLANCNFSM4IK4LXEQ> .

Kavignon · 2019-08-19T20:44:23Z

Then potentially we should remove the CE for maybe since it's not being used in your implementation?

realvictorprm · 2019-08-19T20:46:09Z

I agree, if it's not used anymore by all means! It should be removed :) Unfortunately I'm not at my PC anymore, maybe someone else can apply that few bits, the person just needs to ping me on the OpenTK discord or here in github. Then I'll give the person push rights. Kevin Avignon <notifications@github.com> schrieb am Mo., 19. Aug. 2019, 22:44:

…

Then potentially we should remove the CE for maybe since it's not being used in your implementation? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#946?email_source=notifications&email_token=AGB4Z4HCCMSU3Z4O2Y63RMTQFMA27A5CNFSM4IK4LXE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4UHTTI#issuecomment-522746317>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGB4Z4BUQ5YBU6AVKTWXSFTQFMA27ANCNFSM4IK4LXEQ> .

… of ty as name

Kavignon · 2019-08-20T13:21:10Z

src/Generator/Program.fs

@@ -76,6 +78,8 @@ let autoGenerateAdditionalOverloadForType (func: PrintReadyTypedFunctionDeclarat
                        let (genericName, newParameterType) =
                            currParameter.typ.typ 
                            |> transformPointerTy i tyMapper
+                        // Yes this is a bit evil, I'm sorry but this was easiest here.


Why is this evil?

Improvements!

…GLVULKANPROCNV

…erator

varon · 2019-08-21T20:56:39Z

Thanks so much for this amazing PR, @realvictorprm .

It's a huge, huge contribution and helps set up the project and move it forward in the most amazing way. Special Thanks from everyone at OpenTK!

realvictorprm added 4 commits August 11, 2019 20:05

Started implementation of the specification parser

52655e2

Enum groups for enum types are now referenced in the typed function s…

dc0ec38

…ignature & will notify if a function references an unregistered enum group

initial implementation of code generation

cac93c6

Deduplication attempt

02ba4cb

varon changed the title ~~Feature/new generator~~ [WIP] Feature/new Bindings generator Aug 12, 2019

realvictorprm added 2 commits August 12, 2019 16:18

Fix syntax errors, reserved keywords are now replaced with upper variant

de60fe1

Generate static class and sort out helper functions :)

39a1b83

deccer suggested changes Aug 12, 2019

View reviewed changes

realvictorprm added 2 commits August 12, 2019 22:19

First working experimental version

8856260

Make the functions public so one can use them

4a2311a

deccer suggested changes Aug 14, 2019

View reviewed changes

src/binding/Binding/Initializer.cs Outdated Show resolved Hide resolved

src/binding/Binding/Types.cs Outdated Show resolved Hide resolved

realvictorprm added 8 commits August 14, 2019 23:10

Restructuring and playing around with something. Project won't build …

0809097

…right now, sorry!

Refactored printing slightly

3164b87

Performance optimizations

be32d7b

wip

9714e82

Adjust output file naming, added command line parameters & adapted bu…

6c80a85

…ild script accordingly

Rename DummyType to DummyTypeDescriptor & ignore gl.xml

8d1c1a4

Remove stupid GL.cs file

dcd8754

Remove unneeded files functions.cs & Script1.fsx

950a04c

realvictorprm added 2 commits August 17, 2019 13:00

Function overloads & projects added to the solution

a68f72f

Don't download .NET Core SDK if it's already installed

7739ca3

deccer reviewed Aug 17, 2019

View reviewed changes

src/Generator/Program.fs Outdated Show resolved Hide resolved

deccer reviewed Aug 17, 2019

View reviewed changes

src/Generator/Program.fs Outdated Show resolved Hide resolved

deccer reviewed Aug 17, 2019

View reviewed changes

src/Generator/Program.fs Outdated Show resolved Hide resolved

Try solving issues related to the TP

15f2685

Merge branch 'master' into feature/NewGenerator

72d6019

frederikja163 mentioned this pull request Aug 20, 2019

PlaceholderGL #948

Closed

Apply review, remove dead code, add documentation and use typ instead…

62bdcd0

… of ty as name

Kavignon reviewed Aug 20, 2019

View reviewed changes

Kavignon approved these changes Aug 20, 2019

View reviewed changes

realvictorprm and others added 19 commits August 20, 2019 15:56

Fix formatting of struct constraint

1c04f80

move Generator out of tests folder

cef8add

better type names

9d28e2e

Cleanup addition types generation

14185a9

remove dead code

8eb3604

Replace Either with standard choice type

d35a185

Move utils to top of compilation order

2be04c3

Show fractional time in seconds

e500870

ordering to put output towards the end of project + cleanup

e001f91

Improve bindings generator greeting

2d2749b

targeted optimization

9ace1f5

Change to batch GC mode

8789cc8

start small refactoring via feedback by varon

1fb3f2d

Refactor Program.fs

325a50d

Merge branch 'feature/NewGenerator' into newgenerator

a7cb262

Merge pull request #2 from varon/newgenerator

0a8d2db

Improvements!

Fix renaming of EnumGroup to GLEnumGroup etc.

a9b7742

Fix parsing bug for additional info after name tag & add support for …

62b6c55

…GLVULKANPROCNV

Merge branch 'master' of github.com:opentk/opentk into feature/NewGen…

6716268

…erator

varon merged commit 7b036e5 into opentk:master Aug 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/new Bindings generator #946

Feature/new Bindings generator #946

realvictorprm commented Aug 11, 2019 •

edited

Loading

NogginBops commented Aug 11, 2019 •

edited

Loading

varon commented Aug 12, 2019

realvictorprm commented Aug 12, 2019 via email

deccer Aug 12, 2019

varon Aug 13, 2019

realvictorprm Aug 17, 2019

deccer Aug 12, 2019 •

edited

Loading

deccer Aug 12, 2019

deccer Aug 12, 2019

realvictorprm Aug 17, 2019

deccer Aug 12, 2019

realvictorprm Aug 14, 2019

frederikja163 commented Aug 17, 2019

Kavignon commented Aug 19, 2019

realvictorprm commented Aug 19, 2019 via email

Kavignon commented Aug 19, 2019

realvictorprm commented Aug 19, 2019 via email

Kavignon Aug 20, 2019

varon commented Aug 21, 2019


		let testAssemblies = "tests/*/obj/Release/Tests*.dll"

		let nugetCommandRunnerPath = ".fake/build.fsx/packages/NuGet.CommandLine/tools/NuGet.exe" \|> Fake.IO.Path.convertWindowsToCurrentPath

Feature/new Bindings generator #946

Feature/new Bindings generator #946

Conversation

realvictorprm commented Aug 11, 2019 • edited Loading

Replace binder with a rewritten modern variant

Testing status

Comments

NogginBops commented Aug 11, 2019 • edited Loading

varon commented Aug 12, 2019

realvictorprm commented Aug 12, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deccer Aug 12, 2019 • 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

frederikja163 commented Aug 17, 2019

Kavignon commented Aug 19, 2019

realvictorprm commented Aug 19, 2019 via email

Kavignon commented Aug 19, 2019

realvictorprm commented Aug 19, 2019 via email

Choose a reason for hiding this comment

varon commented Aug 21, 2019

realvictorprm commented Aug 11, 2019 •

edited

Loading

NogginBops commented Aug 11, 2019 •

edited

Loading

deccer Aug 12, 2019 •

edited

Loading