Skip to content
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

[meson] Add needed compiler flags #2240

Merged
merged 1 commit into from
Mar 11, 2020
Merged

Conversation

ebraminio
Copy link
Collaborator

@ebraminio ebraminio commented Mar 11, 2020

Are based on https://github.com/harfbuzz/harfbuzz/blob/2.6.4/configure.ac#L86

@tp-m Can you help on the comments on the patch and review please?

@khaledhosny @behdad just have merged meson port https://github.com/harfbuzz/harfbuzz/pull/1437/files and applied 1c3f80b c494d7a on top, let's reach to some acceptable level at least to remove CMake port before the release, please have a look at the changes and give me and @tp-m your feedbacks, thanks!

@ebraminio ebraminio added the meson meson build system label Mar 11, 2020
meson.build Outdated Show resolved Hide resolved
meson.build Outdated
add_global_arguments('-fno-rtti', language : 'cpp')
add_global_arguments('-fno-exceptions', language : 'cpp')
add_global_arguments('-fno-threadsafe-statics', language : 'cpp')
if (true) # how to check it isn't mingw?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use this for mingw?

Ideally you'd just check if the compiler supports it as above without special-casing.

If special-casing is needed then perhaps if cpp.get_id() = 'gcc' and host_machine.system() == 'windows'

Copy link
Collaborator Author

@ebraminio ebraminio Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use this for mingw?

560d68a and e3320ec

as Windows are by default are trying to hide the symbols by default I guess

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC mingw by default exports all symbols (in general, don't know about inline functions, I would expect those not to be exported), msvc by default exports no symbols.

Personally, I would use the meson port as an opportunity to start over and ignore cruft like that, and only fix it if it still causes issues.

I don't fully understand these commits, probably missing context (didn't look what that script does).

Copy link
Collaborator Author

@ebraminio ebraminio Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I would use the meson port as an opportunity to start over and ignore cruft like that, and only fix it if it still causes issues.

Ok, let's comment it, what do you think about those ARM ones? Comment those as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we need to check for != 'windows' here if we want to do if it not mingw. My bad, sorry.

And you don't need the braces in if (foo and bar) for what it's worth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually:

if host_machine.cpu_family() == 'arm' and cpp.alignment('struct { char c; }') != 1
  add_global_arguments(cpp.get_supported_arguments(['-mstructure-size-boundary=8']), language : 'cpp')
endif

or

if host_machine.cpu_family() == 'arm' and cpp.alignment('struct { char c; }') != 1
  if cpp.has_argument('-mstructure-size-boundary=8')
    add_global_arguments('-mstructure-size-boundary=8', language : 'cpp')
  endif
endif

And one more thing: we shouldn't use add_global_arguments() but add_project_arguments() - the difference matters if harfbuzz is used as a meson subproject. We only want these things to apply to harfbuzz.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. At this point the patch has come more of your work than mine, can I close this so you upload it again? :)

Copy link
Contributor

@tp-m tp-m Mar 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, don't worry about it. I don't need any credit for that, have enough in the history of the meson port. Thanks for handling this instead of just leaving it to me! (unless you'd rather I JFDI of course - which I can do but only later.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so have a look :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

meson.build Show resolved Hide resolved
@ebraminio ebraminio force-pushed the meson branch 5 times, most recently from d8a8cc9 to f7ecf65 Compare March 11, 2020 19:03
@ebraminio ebraminio merged commit 365d2d3 into harfbuzz:master Mar 11, 2020
@ebraminio ebraminio deleted the meson branch March 11, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meson meson build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants