Page MenuHomePhabricator

Meson.build cleanup.
ClosedPublic

Authored by jptiz on Thu, Jun 18, 5:10 PM.

Details

Summary

This is a 8 commit patch, but only for a while - after agreeing with
each of the changes, it shall be squashed into a single commit.

I want to make further changes on meson.build (maybe I can end up simplifying
the build system, or just let things more organized in the end) and thought
that starting with a cleanup would be a good first step.

The changes are:

  1. build: set arguments scope to project instead of globally

    If we set arguments globally, it may conflict with other builds - specially considering meson's subprojects feature. Setting to project arguments ensures we are only considering EFL and not leaking unwanted flags.
  1. build: Fix spacing and indent

    Mostly because it is not well standardized during the file - sometimes there's spaces between tokens, sometimes there is not, etc. The same applies to indent.
  1. build: move test environment closer to test commands

    Just as a matter of organization. If we're doing things for tests that don't impact other stuff, then leave it when tests are handled.
  1. build: Remove unnecessary parenthesis and == true comparisons

    Less noisy redundancy: true is already true, and false is already false, no need to re-check. Besides, reading if sys_windows and if sys_windows == true shouldn't have different effects, as the first you can read as "if the system is windows". It gets better when you have not instead of == false, so for an example you could read if not sys_windows as "if it is not a windows system" more naturally.
  1. build: Switch pc_files to dict

    Just thought it could stay a little better (since it works as a dict), specially in the foreach right after.
  1. [removed to a future patch] build: Use meson's warning_level instead of hardcoded -Wall

    This way we ensure this is compiler-independant (and use the correct feature for that, since meson even warns when configuring the build dir).
  1. build: Use language args from add_project_arguments properly instead of a loop

    The language: kwarg from add_{project,global}_arguments receives a list of languages, so no need for that loop.
  1. [removed to a future patch] build: Use '/' instead of join_paths

    As it is recommended by meson since v0.49 (and stays clearer IMO, specially since that's how some languages are adopting path separation, e.g. C++'s filesystem stdlib).

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jptiz created this revision.Thu, Jun 18, 5:10 PM
jptiz requested review of this revision.Thu, Jun 18, 5:10 PM
bu5hm4n requested changes to this revision.Fri, Jun 19, 1:51 AM

1 / 2 / 3 / 4: fine for me.
We recently bumped min version to 0.50 so 5 & 7 Should hopefully work without issues.

6: Mhm, -Wall does not find its way into the cflags of the build.ninja file, even if the default warning is 3. That should be fixed.

8: That is one of those things where opinions clash. You can say here that the fact that 'a' / 'b' != ' a / b' is rather ugly and error prone. There are projects that do migrate to / over join_paths, others dont, i dont really care in which direction we go. However, if we switch there it might makes sense to do it over all occurences. (Not necessarily in this patch). Maybe we should talk about that in some time on irc ? (so others can join in)

This revision now requires changes to proceed.Fri, Jun 19, 1:51 AM
vtorri added a subscriber: vtorri.Fri, Jun 19, 2:04 AM

for 8, i am not against /. Windows file system understands / as path separator, so same path separator for all the OS

though i would have liked several patch instead of a long patch with unrelated diff. Better to find errors with bisect

jptiz added a comment.EditedFri, Jun 19, 7:43 AM

1 / 2 / 3 / 4: fine for me.
We recently bumped min version to 0.50 so 5 & 7 Should hopefully work without issues.

Yay

6: Mhm, -Wall does not find its way into the cflags of the build.ninja file, even if the default warning is 3. That should be fixed.

Hm, didn't realize that, gonna check it.

8: That is one of those things where opinions clash. You can say here that the fact that 'a' / 'b' != ' a / b' is rather ugly and error prone. There are projects that do migrate to / over join_paths, others dont, i dont really care in which direction we go. However, if we switch there it might makes sense to do it over all occurences. (Not necessarily in this patch). Maybe we should talk about that in some time on irc ? (so others can join in)

Makes sense. Gonna call you there then.

for 8, i am not against /. Windows file system understands / as path separator, so same path separator for all the OS

though i would have liked several patch instead of a long patch with unrelated diff. Better to find errors with bisect

Makes sense².

With these two comments, then: should I split 6 and 8 into two other patches? For 6 because we can track this in an isolated patch, and for 8 because of possible opinion divergence.

With these two comments, then: should I split 6 and 8 into two other patches? For 6 because we can track this in an isolated patch, and for 8 because of possible opinion divergence.

Sounds good :) (If getting out 8 is too much work, i dont have a issue with first merging that if the rest of the stuff is coming in a next patch :))

vtorri added inline comments.Thu, Jun 25, 4:29 AM
meson.build
108

why did you remove DLL_EXPORT ?

jptiz added a comment.Thu, Jun 25, 9:16 AM

With these two comments, then: should I split 6 and 8 into two other patches? For 6 because we can track this in an isolated patch, and for 8 because of possible opinion divergence.

Sounds good :) (If getting out 8 is too much work, i dont have a issue with first merging that if the rest of the stuff is coming in a next patch :))

I'm gonna give a try, but I don't think it should be so much work (just too many files, tho :'D). Gonna split'em then :)

meson.build
108

Oops! Little accident while rebasing :D Thanks for pointing out.

jptiz updated this revision to Diff 30787.Thu, Jun 25, 12:16 PM

Add back the accidentally-removed DLL_EXPORT=1.

jptiz added a comment.EditedTue, Jun 30, 8:05 PM

6: Mhm, -Wall does not find its way into the cflags of the build.ninja file, even if the default warning is 3. That should be fixed.

About this, two things:

  • Ensure you --wipe the build dir (or start from a clean one), since the change is on default option, but on an old build dir it wouldn't change since the dir would have its own previous definitions...including the older warning_level whose value's probably going to be the previous default. In this case, no warning flag at all will be issued by default.
  • After some more debugging I finally understood why: Meson doesn't send warn_args on buildtype=plain builds (https://github.com/mesonbuild/meson/blob/d42cd735a4dc894d8e898a5f9e81029f6eb5364c/mesonbuild/backend/backends.py#L641), which is EFL's default. That's the reason for this. In fact, Meson doesn't send any of its args at all when in plain build, as stated here:

The command line switch --buildtype=plain tells Meson not to add its own flags to the command line. This gives the packager total control on used flags.

But I don't know if it plain should be EFL's default. I'm going to split that part anyway, since it'll need a little more reasoning and I just discovered - because of the alternative to D12034 - that I need this a little faster than I thought haha (because of add_global_arguments giving errors when EFL's a subproject).

jptiz updated this revision to Diff 30808.Tue, Jun 30, 8:22 PM

Removed 6 and 8 from this patch, so we stick with just:

  • build: Set arguments scope to project instead of globally
  • build: Fix spacing and indent
  • build: move test environment closer to test commands
  • build: Remove unnecessary parenthesis and == true comparisons
  • build: Switch pc_files to dict
  • build: Use language args from add_project_arguments properly instead of a loop
jptiz edited the summary of this revision. (Show Details)Tue, Jun 30, 8:24 PM
jptiz edited the summary of this revision. (Show Details)
jptiz added a comment.Tue, Jun 30, 8:33 PM

Ok, I think that's it for this patch. Should I squash the commits it or will it be squashed by who lands the patch?

bu5hm4n accepted this revision.Wed, Jul 1, 1:13 AM

thx.

This revision is now accepted and ready to land.Wed, Jul 1, 1:13 AM
Closed by commit rEFL98fd37e76878: Meson.build cleanup. (authored by jptiz, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyWed, Jul 1, 1:14 AM
This revision was automatically updated to reflect the committed changes.