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

gnome: add mkenums_simple() #1800

Merged
merged 1 commit into from Aug 14, 2017

Conversation

Projects
None yet
6 participants
@tp-m
Member

tp-m commented May 14, 2017

99% of all mkenums uses in C libraries use the same basic template,
so add a mkenums_simple() function that takes care of everything for
us based on that template.

Features:

  • optional function declaration decorator such as GLIB_AVAILABLE
  • optional extra header prefix (e.g. for include needed for decorator)
  • optional function prefix (e.g. to add leading underscores)

Fixes issue #1384

static const G@Type@Value values[] = {''' % func_prefix
c_file_kwargs['vprod'] = \
' { C_@TYPE@(@VALUENAME@), "@VALUENAME@", "@valuenick@" },'

This comment has been minimized.

@ignatenkobrain

ignatenkobrain May 14, 2017

Member

[flake8]

  • [E128] continuation line under-indented for visual indent

@tp-m tp-m force-pushed the tp-m:gnome-mkenums-simple branch from a6301b3 to f5eb5bb May 14, 2017

@TingPing

This comment has been minimized.

Member

TingPing commented May 14, 2017

So what is the reason this has to be a new API?

@TingPing TingPing added the gnome label May 14, 2017

@tp-m

This comment has been minimized.

Member

tp-m commented May 14, 2017

Well, it doesn't have to be new API, but squeezing it into the existing mkenums seemed to just make everything more horrible and confusing from a user point of view. It's already not very nice API because it's so overloaded IMHO.

How would you add this functionality to the existing mkenums? Maybe you can think of a nicer way :)

@tp-m

This comment has been minimized.

Member

tp-m commented May 14, 2017

Actually, I would probably argue that they are different things. mkenums maps pretty much 1:1 to glib-mkenums, so is really just a way to call it from meson with the arguments it provides. The proposed mkenums_simple on the other hand is meant as a higher-level abstraction.

@TingPing

This comment has been minimized.

Member

TingPing commented May 14, 2017

Well just glancing at the docs you wrote like 80% of them overlap. I just think it reflects upon Meson poorly that our API is so bad we already have a duplicate entry with _simple() shoved on the end.

@tp-m

This comment has been minimized.

Member

tp-m commented May 14, 2017

Well, we can't undo the past, can we? I think we should always be guided by providing the best usability for our users going forward, irrespective of how this may or may not reflect on meson :)

@nirbheek

This comment has been minimized.

Member

nirbheek commented May 15, 2017

I don't think our API is bad, it's just that mkenums API is extremely flexible and we chose to expose that first. Later, based on the most-common usage, we've distilled 90% of the use-cases into one mkenums_simple call that everyone should be able to use without specifying templates or fhead, etc.

@nirbheek

I've gone over the code, but I can't do a proper technical review of this since @TingPing is the expert here.

body_filename = args[0] + '.c'
# not really needed, just for sanity checking
forbidden_kwargs = ['c_template', 'h_template', 'eprod', 'fhead',

This comment has been minimized.

@nirbheek

nirbheek May 15, 2017

Member

Maybe make this a whitelist instead of a blacklist? Similar to mkenums() itself.

This comment has been minimized.

@tp-m

tp-m May 15, 2017

Member

The main purpose of this was to catch old-api arguments from people who migrate from mkenums to mkenums_simple, to make sure there is no leakage. I can also remove this entirely. Our implementation calls the mkenums implementation in the end, so things will be checked there too.

This comment has been minimized.

@tp-m

tp-m May 15, 2017

Member

Actually, what I wrote makes no sense because I only copy over specific fields via a whitelist already, so there can be no leakage. Just unused args in meson.build file.

This comment has been minimized.

@nirbheek

nirbheek May 15, 2017

Member

Yes, but a whitelist will also protect against typos of kwargs that we do accept, that's all.

@tp-m

This comment has been minimized.

Member

tp-m commented May 15, 2017

I don't think the code contains anything particularly technically challenging, since we re-use the existing mkenums implementation for all the important bits.

@nirbheek

This comment has been minimized.

Member

nirbheek commented May 31, 2017

Is there anything else we need to discuss or change here?

@tp-m

This comment has been minimized.

Member

tp-m commented May 31, 2017

It needs to be used and tested with various modules to make sure it fulfils their needs and does everything we want it to.

I want to convert gst-plugins-base at least. I also tried to use it in pango but it caused some weird issue which I didn't have time to debug yet.

@tp-m

This comment has been minimized.

Member

tp-m commented Jul 31, 2017

@alessandrod has successfully converted various GStreamer modules over to this (base+bad), so should be alright.

install: true)
```
*Added 0.41.0*

This comment has been minimized.

@TingPing

TingPing Aug 2, 2017

Member

Bump version

Generates enum `.c` and `.h` files for GObject using the `glib-mkenums` tool
with the standard template used by most GObject-based C libraries. The first
argument is the base name of the output files.

This comment has been minimized.

@TingPing

TingPing Aug 2, 2017

Member

In gnome.mkenums you should probably reference this function and explain why you might want to use this.

This comment has been minimized.

@tp-m

tp-m Aug 2, 2017

Member

Thanks for the review, updated.

@tp-m tp-m force-pushed the tp-m:gnome-mkenums-simple branch from f5eb5bb to e4ee151 Aug 2, 2017

@tp-m tp-m changed the title from gnome: add mkenums_simple() (WIP DO NOT MERGE) to gnome: add mkenums_simple() Aug 4, 2017

@tp-m tp-m added this to the 0.42.0 milestone Aug 4, 2017

Example:
```

This comment has been minimized.

@TingPing

TingPing Aug 4, 2017

Member

Specify the meson language for syntax highlighting

@@ -93,6 +99,49 @@ Note that if you `#include` the generated header in any of the sources for a bui
Returns an array of two elements which are: `[c_source, header_file]`
### gnome.mkenums_simple()

This comment has been minimized.

@TingPing

TingPing Aug 4, 2017

Member

Add notes about the new API in docs/markdown/Release-notes-for-0.42.0.md

fhead += '#include "%s"\n' % hdr_filename
for hdr in sources:
fhead += '#include "%s"\n' % hdr
fhead += '''

This comment has been minimized.

@TingPing

TingPing Aug 4, 2017

Member

All these empty newlines show up in the generated file.

EDIT: Actually this specific newline is fine, but before the includes is 4 empty lines.

@tp-m tp-m force-pushed the tp-m:gnome-mkenums-simple branch from e4ee151 to c7b0e06 Aug 5, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Aug 5, 2017

Codecov Report

Merging #1800 into master will increase coverage by 7.33%.
The diff coverage is 80.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1800      +/-   ##
==========================================
+ Coverage   56.47%   63.81%   +7.33%     
==========================================
  Files          74       74              
  Lines       18192    18238      +46     
  Branches     3787     3796       +9     
==========================================
+ Hits        10274    11638    +1364     
+ Misses       6727     5416    -1311     
+ Partials     1191     1184       -7
Impacted Files Coverage Δ
mesonbuild/modules/gnome.py 60.51% <80.43%> (+1.12%) ⬆️
mesonbuild/dependencies/ui.py 66.02% <0%> (+0.24%) ⬆️
mesonbuild/mesonlib.py 81.92% <0%> (+0.39%) ⬆️
mesonbuild/mtest.py 72.46% <0%> (+1.32%) ⬆️
mesonbuild/compilers/fortran.py 68.75% <0%> (+1.7%) ⬆️
mesonbuild/compilers/compilers.py 81.32% <0%> (+1.81%) ⬆️
mesonbuild/mintro.py 54.73% <0%> (+2.1%) ⬆️
mesonbuild/modules/__init__.py 90.14% <0%> (+2.81%) ⬆️
mesonbuild/coredata.py 72.76% <0%> (+3.82%) ⬆️
mesonbuild/backend/ninjabackend.py 88.1% <0%> (+3.85%) ⬆️
... and 17 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 55165ba...73e5047. Read the comment docs.

@tp-m tp-m force-pushed the tp-m:gnome-mkenums-simple branch from c7b0e06 to 1946a50 Aug 5, 2017

# instead, but that seems like much more work, nice as it would be.
fhead = ''
if body_prefix != '':
fhead += '%s\n' % body_prefix

This comment has been minimized.

@ignatenkobrain

ignatenkobrain Aug 5, 2017

Member

[flake8]

  • [E111] indentation is not a multiple of four

@tp-m tp-m force-pushed the tp-m:gnome-mkenums-simple branch from 1946a50 to 1d706b2 Aug 5, 2017

@tp-m

This comment has been minimized.

Member

tp-m commented Aug 5, 2017

Thanks, updated. I believe the remaining three newlines at the beginning of the generated files are glib-mkenum's fault?

@TingPing

This comment has been minimized.

Member

TingPing commented Aug 5, 2017

I believe the remaining three newlines at the beginning of the generated files are glib-mkenum's fault?

It is possible; extra whitespace isn't a blocker but it would be nice to fix upstream. Anyway works well here 👍

@nirbheek

This comment has been minimized.

Member

nirbheek commented Aug 13, 2017

Conflicts, and the windows build needs to be re-triggered; something went weird with the Cygwin build.

@tp-m tp-m force-pushed the tp-m:gnome-mkenums-simple branch from 1d706b2 to ff0dea8 Aug 13, 2017

gnome: add mkenums_simple()
99% of all mkenums uses in C libraries use the same basic template,
so add a mkenums_simple() function that takes care of everything for
us based on that template.

Features:
 - optional function declaration decorator such as GLIB_AVAILABLE
 - optional extra header prefix (e.g. for include needed for decorator)
 - optional extra body prefix (e.g. for additional includes)
 - optional function name prefix (e.g. to add leading underscores)

Fixes issue #1384

@tp-m tp-m force-pushed the tp-m:gnome-mkenums-simple branch from ff0dea8 to 73e5047 Aug 14, 2017

@tp-m

This comment has been minimized.

Member

tp-m commented Aug 14, 2017

Fixed conflicts, and then again :)

@jpakkane

This comment has been minimized.

Member

jpakkane commented Aug 14, 2017

Everyone happy with this now?

@jpakkane jpakkane merged commit 4e476c8 into mesonbuild:master Aug 14, 2017

3 checks passed

ci/sideci Meow! No issues found. It's clean code!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment