-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add OS/2 support #14106
base: master
Are you sure you want to change the base?
Add OS/2 support #14106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're currently in a feature freeze, so this won't be able to land for a bit.
I've given this a first pass over, though I suspect a few iterations will be needed before this lands. Thanks for working on this
mesonbuild/compilers/mixins/clike.py
Outdated
for s in suffixes: | ||
patterns.append(p + '{}.' + s) | ||
for p in prefixes: | ||
if s.startswith('_dll.') or s.startswith('_s.'): | ||
patterns.append(p + '{}' + s) | ||
else: | ||
patterns.append(p + '{}.' + s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be cleaned up a little and fold the cases together:
for s in suffixes
dot = '.' if env[machines][self.for_machine].is_os2() and s.startswith(('_dll.', '_s.'))
for p in prefixes:
patterns.append('p + '{}' + dot + s)
edit: fixed copy/paste error in the code snippet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last ':' of if statement is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, no, that was a copy/paste error on my part. I'll edit it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I had to :
to the end of 'for' statement line, remove '
in fromt of p
at the last line.
mesonbuild/compilers/detect.py
Outdated
@@ -165,6 +168,8 @@ def detect_static_linker(env: 'Environment', compiler: Compiler) -> StaticLinker | |||
trials = [defaults['cuda_static_linker']] + default_linkers | |||
elif compiler.get_argument_syntax() == 'msvc': | |||
trials = [defaults['vs_static_linker'], defaults['clang_cl_static_linker']] | |||
elif is_os2() and env.coredata.get_option(OptionKey('emxomf')): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elif is_os2() and env.coredata.get_option(OptionKey('emxomf')): | |
elif env.machines[compiler.for_machine].is_os2() and env.coredata.get_option(OptionKey('emxomf')): |
is_os2()
will give the wrong answer in a cross compile. I've been meaning to delete all of those for a while so that we have to use the Environment.machines
ones, but never seem to get there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean I should replace all is_os2()
with Environment.machines ones ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the is_os2()
function is only safe to use if you know that it will always be used for the build machine, so, yes please, use Environment.machines
if it's available. (There are probably some places we should use that but don't have it available, I'm fine with ignoring those because they're per-exisiting issues)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll try.
mesonbuild/compilers/compilers.py
Outdated
if mesonlib.is_os2(): | ||
args += ['-Zomf'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should go in the Compiler.get_always_args
and Linker.get_always_args
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to move those codes to GnuCompiler. However, I don't know how to access to the global options. How can I do that?
mesonbuild/build.py
Outdated
shortname = kwargs.get('shortname') | ||
if shortname: | ||
self.shortname = shortname |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you can't set this when you initially set self.shortname
, like self.shortname = kwargs.get('shortname')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. Maybe I followed the coding style around.
mesonbuild/backend/ninjabackend.py
Outdated
if mesonlib.is_os2(): | ||
rule = '{}_DLL_LINKER{}'.format(langname, self.get_rule_suffix(for_machine)) | ||
command = compiler.get_linker_exelist() | ||
args = ['$ARGS'] + NinjaCommandArg.list(compiler.get_linker_output_args('$out'), Quoting.none) + ['$in', '$LINK_ARGS'] | ||
args += ['&&', 'emximp', '-o', '$IMPLIB', '$out'] | ||
|
||
description = 'Linking target $out' | ||
if num_pools > 0: | ||
pool = 'pool = link_pool' | ||
else: | ||
pool = None | ||
|
||
options = self._rsp_options(compiler) | ||
self.add_rule(NinjaRule(rule, command, args, description, **options, extra=pool)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you always end up using this rule for dynamic linking on OS/2 paths, am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in order to build an import library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So can we just have the normal shared_library linking path do this on OS/2, or is there a use of that I missed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to do so, but I'm sorry I have no idea how to do. Any hints ?
@@ -2167,14 +2167,16 @@ def post_init(self) -> None: | |||
# See our FAQ for more detailed rationale: | |||
# https://mesonbuild.com/FAQ.html#why-does-building-my-project-with-msvc-output-static-libraries-called-libfooa | |||
if not hasattr(self, 'prefix'): | |||
self.prefix = 'lib' | |||
self.prefix = '' if self.environment.machines[self.for_machine].is_os2() else 'lib' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this violate the rational for why we use 'lib
as a prefix on Windows, even though that's not the standard thing to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If meson build system requires 'lib' prefix for a static lib, I'll do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above points to https://mesonbuild.com/FAQ.html#why-does-building-my-project-with-msvc-output-static-libraries-called-libfooa which describes the reasoning:
- On Windows, we need to use .a in order to distinguish between import libraries (foo.lib) and static libraries (which we name with
lib*.a
to resolve the name clash). - it works well with the mingw toolchain, that can look this up on Windows using
-lfoo
How does OS/2 handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On OS/2, there are no official distinctions between static libs and an import libs. However, conventionally gcc + kLIBC uses _dll.a
and _dll.lib
for import libs, _s.a
and _s.lib
for static libs. .a
and .lib
can be used for both. If .a
and .lib
are paired with _dll.a
and _dll.lib
, they are static libs. If paired with _s.a
and _s.lib
, they are import libs.
And linkers search libraries with -lfoo
in the following order by default:
1. libfoo_dll.a
2. foo_dll.a
3. libfoo.a
4. foo.a
5. foo.dll (this requires additional option)
If -Zomf
is used, .lib
is searched prior to .a
in every search order.
.a
libs including _dll.a
and _s.a
are for aout format which are created by ar
and linked by ld
, but .lib
libs including _dll.lib
and _s.lib
are for OMF format which are created created by emxomfar
and linked by emxomfld
.
@@ -112,7 +112,7 @@ class DFeatures(TypedDict): | |||
cs_kwargs) | |||
|
|||
known_exe_kwargs = known_build_target_kwargs | {'implib', 'export_dynamic', 'pie', 'vs_module_defs'} | |||
known_shlib_kwargs = known_build_target_kwargs | {'version', 'soversion', 'vs_module_defs', 'darwin_versions', 'rust_abi'} | |||
known_shlib_kwargs = known_build_target_kwargs | {'version', 'soversion', 'vs_module_defs', 'darwin_versions', 'rust_abi', 'shortname'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'shortname' needs to be added to the documentation for shared_library in the docs section.
Additionally, in interpreter/type_checking.py you need to add:
KwargInfo('shortname', (str, NoneType), since='1.8')
to the _EXCLUSIVE_SHARED_LIB_KWS
list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll try.
@@ -647,6 +648,7 @@ def add_to_argparse(self, name: str, parser: argparse.ArgumentParser, help_suffi | |||
(OptionKey('wrap_mode'), BuiltinOption(UserComboOption, 'Wrap mode', 'default', choices=['default', 'nofallback', 'nodownload', 'forcefallback', 'nopromote'])), | |||
(OptionKey('force_fallback_for'), BuiltinOption(UserArrayOption, 'Force fallback for those subprojects', [])), | |||
(OptionKey('vsenv'), BuiltinOption(UserBooleanOption, 'Activate Visual Studio environment', False, readonly=True)), | |||
(OptionKey('emxomf'), BuiltinOption(UserBooleanOption, "Whether to use OMF format on OS/2", False)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about configuring the object format from a global option, I need to think more about this, but it might be more appropriate as a compiler option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, it would be coolI if possible to change the object format without modifying meson.build file. And it's possible to prepare compiler options depending on the global option.
On OS/2, 1. a shared libary has '.dll' suffix 2. a length of DLL name should not be longer than 8 chars 3. an import library is used to link against a DLL 4. an import library has '_dll.a' suffix.
Give an user opportunities to mangle a custom short name for a DLL on OS/2.
1. Generate OMF objs with `-Zomf' compiler flags 2. Generate OMF libs with `.lib' suffix using `emxomfar' as a librarian
I've done except #14106 (comment), #14106 (comment), #14106 (comment) and #14106 (comment) |
Hi/2.
This PR is to support OS/2.
Review, please...