Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

Attempt at updating Clang scripts #535

Merged
merged 12 commits into from
Jan 14, 2021

Conversation

coloursofnoise
Copy link
Contributor

I took a stab at going through and semi-blindly updating the generator scripts to use the new API provided by Clang.jl.
While I'm relatively sure that it all works, from what I can tell it was originally set up to work on Linux, and I have been so far unsuccessful in running them on my Windows machine.

Issues so far (occurring in gtk_auto_gen.jl):

  • gtk_libpaths = ("/opt/local", "/usr/lib") obviously neither of these paths are present on Windows, this is fixed by setting header to the path of your local gtk.h file.
  • args = read(`pkg-config --cflags gtk+-$gtk_version.0`, String) no clue where pkg-config is supposed to be, I just replace this with args = " ".
  • "fatal error: vcruntime.h file not found": parse_header takes an optional includes argument which should be supplied with folders containing any missing header files
  • "(...)/vadefs.h error: expected ; after top level declarator: This is where I'm stuck, it seems as though Clang is unable to properly parse some header files, despite them being the ones installed as part of the Visual Studio MSVC.

It's entirely possible that I'm wrong about this and that the scripts need to be partially or completely rewritten, but if someone could test this out on another machine/OS it would be greatly appreciated.

@tknopp
Copy link
Collaborator

tknopp commented Nov 26, 2020

Great, thank! Two thoughts:

  • I do not think that it is fully critical getting these scripts running on windows. I use Linux and OS X and would be more than happy if they would at least run there.
  • In order to not let these scripts bitrot it would be great if we would run them (somehow) in the tests. Do you think this would be feasible? Of course we would just let them run on linux. The generated scripts could then remain in test/ and one could even make comparisons with the files in gen.

@coloursofnoise
Copy link
Contributor Author

It would definitely be possible, my main concern would be making sure it can reliably find the header files it needs, which as far as I know can't really be guaranteed unless they're packaged with Gtk.jl.
If we can find a setup that works on a reasonable number of machines and we can determine when it errors on that step, then we could probably just let it fail silently, and only warn if it succeeds and the generated files don't match those in gen, or if it errors on another step.

@tknopp
Copy link
Collaborator

tknopp commented Nov 27, 2020

yes, whatever we do more than we currently do is better. In my perspective the tests can just run under a very specific setup (i.e. ubuntu on Travis where we have control, if we want gtk header files installed). We could even default to disable the test and only enable it if there is a certain environment variable set. In the Travis script we then can set this variable so that the test is only run there.

@coloursofnoise
Copy link
Contributor Author

That sounds reasonable, unfortunately I wouldn't really be able to implement it since I don't currently have access to a Linux machine.
If you're able to do that it would be great, otherwise I suppose I can just leave this here and hope someone will pick it up.

@giordano
Copy link
Contributor

giordano commented Dec 11, 2020

gtk_libpaths = ("/opt/local", "/usr/lib") obviously neither of these paths are present on Windows, this is fixed by setting header to the path of your local gtk.h file.

Why don't you use GTK3_jll which provides a consistent way to access the header files across all operating systems, and it's also consistent with what this package actually uses?

@tknopp
Copy link
Collaborator

tknopp commented Dec 15, 2020

Why don't you use GTK3_jll which provides a consistent way to access the header files across all operating systems, and it's also consistent with what this package actually uses?

Thats a very good suggestion. The "why" is probably because all this was setup in a time where BB was not a thing ;-)

Use BinaryBuilder packages for header file locations
@coloursofnoise
Copy link
Contributor Author

I have now got a mostly working version, using the BinaryBuilder.jl packages as suggested by @giordano.
I have attached the files generated by these scripts, which still have a couple issues:

  • somehow the beginning of gbox3 needs to be changed from
$(Expr(:import, :., :., :Gtk))
$(Expr(:import, :., :., :Gtk, :GObject))

to

import ..Gtk
import ..Gtk.GObject

I have no idea how this is done, other than that it may be related to Base.show_unquoted so if someone could point me in the right direction it would be much appreciated.

gconsts3 is still missing around half of its definitions, which were presumably generated by gi_gen_consts.jl. I don't know whether using GI is still necessary, as mentioned here but for the time being I will take a look at getting it working. Advice is again very welcome.

Attachments (.txt extension required by GitHub):
gbox3.txt
gconsts3.txt

@tknopp
Copy link
Collaborator

tknopp commented Dec 27, 2020

The history was like this. First there were only the Clang scripts. Then the GI script was added, which potentially could replace all what Clang can do. Then GI did not get migrated to any of the new Julia versions since v0.4(? not exactly sure about the version.)

I am not 100% sure what would be the best strategy forward. There are 2 potential possibilities:

  1. Migrate GI.jl
  2. Try to find out what constanst Clang was not capable of generating and try looking if it is possible to workaround this.

In the long run it would certainly better to use GI.jl and use it much more extensively. GI is what should actually be used by Gtk language bindings: https://gi.readthedocs.io/en/latest/

@coloursofnoise
Copy link
Contributor Author

I've managed to get the Clang scripts working to produce (almost) everything, including what was previously done by GI.jl.
I have also gotten the GI script working on latest, however it is not included in this commit.

Remaining issues:

  • Get functions that take a GValue pointer to the location where the return value will be set are not handled properly, and can cause a conflict by having the same definition as the associated Set function.
  • gtk_drag_source_get_target_list conflicts with gtk_drag_dest_get_target_list (and the same for the set equivalents) because they both generate a function called target_list.
  • an error can occur about missing some std headers which can be fixed by setting STD_INCLUDE to the path to your platform specific stdlib include. On windows there is a julia artifact containing these, but I do not know how or if it can be retrieved programmatically.
    Currently is error is logged and ignored, however the script can be tweaked to instead segfault if that would be preferred.

Generated files (Win7 64bit):
gbox3.txt
gconsts3.txt

@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #535 (9fd2f41) into master (9767ac1) will decrease coverage by 2.33%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
- Coverage   49.94%   47.60%   -2.34%     
==========================================
  Files          32       32              
  Lines        2515     2382     -133     
==========================================
- Hits         1256     1134     -122     
+ Misses       1259     1248      -11     
Impacted Files Coverage Δ
src/GLib/gtype.jl 77.29% <0.00%> (+1.86%) ⬆️
src/Gtk.jl 95.83% <ø> (+6.54%) ⬆️
src/gtktypes.jl 73.33% <0.00%> (-5.24%) ⬇️
src/GLib/gerror.jl 33.33% <0.00%> (-6.67%) ⬇️
src/events.jl 44.18% <0.00%> (-6.36%) ⬇️
src/cairo.jl 70.00% <0.00%> (-4.29%) ⬇️
src/lists.jl 55.28% <0.00%> (-4.08%) ⬇️
src/text.jl 38.48% <0.00%> (-3.44%) ⬇️
src/layout.jl 23.86% <0.00%> (-3.32%) ⬇️
src/container.jl 44.44% <0.00%> (-3.06%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9767ac1...9fd2f41. Read the comment docs.

@coloursofnoise
Copy link
Contributor Author

The only remaining difference from the original generated files is the argument type for gtk_tree_model_get_value iter changing from Ref{Gtk.GtkTreeIter} to Ptr{Gtk.GtkTreeIter}.
I since I believe the change to Ref was done manually, the scripts should now be ready for use.

I have added some hard-coded values for system-dependent constants, such as path separators and module suffixes.

For the time being, any duplicate method definitions in gbox are ignored. Since these scripts should eventually be replaced by GI.jl, I think this is a reasonable compromise for getting them working temporarily, and it currently does not break any preexisting behaviours.

Generated files:
gbox3.txt
gconsts3.txt

@coloursofnoise coloursofnoise marked this pull request as ready for review January 1, 2021 11:37
gen/gtk_auto_gen.jl Outdated Show resolved Hide resolved
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@tknopp
Copy link
Collaborator

tknopp commented Jan 2, 2021

@coloursofnoise: Thanks for working on this!

If I get it right you PR currently has not yet changed the generated files. Have you tried running the tests with the newly generated files?

@coloursofnoise
Copy link
Contributor Author

I have been testing regularly on Win7 and a Debian vm, as well as manually comparing the generated files.
I've been avoiding committing the updated files for ease of comparing them, but I can do that if you'd like

gen/gtk_consts_gen.jl Outdated Show resolved Hide resolved
@tknopp tknopp merged commit 8742856 into JuliaGraphics:master Jan 14, 2021
@tknopp
Copy link
Collaborator

tknopp commented Jan 14, 2021

@coloursofnoise I merged this. Thank you very much working on this. IMHO we still want to run the gen scripts within the tests if possible. This can be done by adding Clang as a dependency only for testing.

@Vexatos
Copy link
Contributor

Vexatos commented Jan 14, 2021

Perhaps it would be a good idea to make a separate Project.toml file inside gen/ to have an environment for the gen scripts that CI could use.

@tknopp
Copy link
Collaborator

tknopp commented Jan 14, 2021

yes, certainly this is also a good idea. But the environment in the test folder should also include Clang if we want to execute them.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants