-
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 support for building Apple bundles #14121
base: master
Are you sure you want to change the base?
Conversation
I would make it a new module. |
The app_bundle function? I can probably do that. I looked into it originally but then didn't do it because this is 90% the same as executable() and also this way you can use build_target to switch between executable and app_bundle depending on build configuration without having to duplicate the target definition. What's your alternative for using build_target like that? |
87b098e
to
d76708a
Compare
d76708a
to
ec73cc4
Compare
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.
Not familiar enough with Apple bundles to say much about the topic, but I have a few observations regarding the coding style.
mesonbuild/build.py
Outdated
def __init__( | ||
self, | ||
name: str, | ||
subdir: str, | ||
subproject: SubProject, | ||
for_machine: MachineChoice, | ||
sources: T.List['SourceOutputs'], | ||
structured_sources: T.Optional[StructuredSources], | ||
objects: T.List[ObjectTypes], | ||
environment: environment.Environment, | ||
compilers: T.Dict[str, 'Compiler'], | ||
kwargs, | ||
): | ||
self.bundle_resources: T.Optional[StructuredSources] = kwargs['bundle_resources'] |
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 not use the "black" deformatter.
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.
Which formatter should I use?
mesonbuild/build.py
Outdated
super().__init__( | ||
name, | ||
subdir, | ||
subproject, | ||
for_machine, | ||
sources, | ||
structured_sources, | ||
objects, | ||
environment, | ||
compilers, | ||
kwargs, | ||
) |
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.
Same.
mesonbuild/build.py
Outdated
def contents_root(self) -> str: | ||
return { | ||
'oldstyle': [], | ||
'contents': ['Contents'], | ||
'flat': [], | ||
}[self.bundle_layout] | ||
|
||
def bin_root(self) -> T.List[str]: | ||
return { | ||
'oldstyle': [], | ||
'contents': [x for x in ['Contents', self.exe_dir_name] if x], | ||
'flat': [], | ||
}[self.bundle_layout] | ||
|
||
def resources_root(self) -> T.List[str]: | ||
return { | ||
'oldstyle': ['Resources'], | ||
'contents': ['Contents', 'Resources'], | ||
'flat': [], | ||
}[self.bundle_layout] | ||
|
||
def info_root(self) -> T.List[str]: | ||
return { | ||
# The only case where Info.plist is in the Resources directory. | ||
'oldstyle': ['Resources'], | ||
'contents': ['Contents'], | ||
'flat': [], | ||
}[self.bundle_layout] |
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.
Surely there must be a better way to do 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.
Yeah, this is super ugly. I do want to find a better way to write this. Could at least make this return a str and keep the rest the same but I'm still not satisfied with that.
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.
There, I suppose the simplest way is probably the best here.
mesonbuild/envconfig.py
Outdated
""" | ||
Machine is Darwin (iOS/tvOS/OS X)? | ||
""" | ||
return self.system in {'darwin', 'ios', 'tvos'} | ||
return self.system == 'darwin' |
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 will probably break existing use cases in the wild. It also looks like it conflicts with changes you made elsewhere? What is the purpose of the change -- the commit message doesn't have much in the way of "why"s and definitely doesn't mention this change at all.
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.
Yeah, I still need to split this change out. From what I can see the system is never 'ios' or 'tvos' but always 'darwin', those variants are subsystem instead (e.g. see cross/iphone.txt).
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 was first added in f59786e. I suppose people are supposed to be able to both use system='ios' and system='darwin' subsystem='ios'? I'll revert this then.
def func_app_bundle( | ||
self, node: mparser.BaseNode, args: T.Tuple[str, SourcesVarargsType], kwargs: kwtypes.BuildTarget | ||
) -> build.AppBundle: |
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 black deformatter strikes again...
mesonbuild/build.py
Outdated
sources: T.List['SourceOutputs'], | ||
structured_sources: T.Optional[StructuredSources], | ||
objects: T.List[ObjectTypes], | ||
environment: environment.Environment, | ||
compilers: T.Dict[str, 'Compiler'], |
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.
When adding brand new code, avoid quoting annotations. Since meson moved to python 3.7 as the minimum, we started using from __future__ import annotations
which has the equivalent effect, so the manual quoting is no longer needed -- and makes lines slightly longer, which adds up and eventually tends to cause overly long lines which then need wrapping.
mesonbuild/build.py
Outdated
def type_suffix(self): | ||
return "@app" |
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.
Can add trivial type annotation. Also, double quotes -> single quotes.
mesonbuild/backend/ninjabackend.py
Outdated
@@ -14,6 +14,7 @@ | |||
import json | |||
import os | |||
import pickle | |||
import plistlib |
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 import is only needed on a single platform, and only some of the time. Maybe it would be a good idea to delay importing it until the function that uses 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.
This can be used on any platform (specifically I also want a reasonable build system for GNUstep on Linux). But yeah, I'll move the import into the function.
ec73cc4
to
284e328
Compare
6e7e637
to
7976bcc
Compare
I have been thinking about this also and maybe having a top level function for this is not the best thing to do. Because in practice it would mean that most build files just end up with One alternate way of doing this would be to have a new option |
Yeah, this is why I asked about using build_target with types added by a module in #14121 (comment). I wouldn't want that if/else either because that's awful to use. Speaking of that, I just thought about a possible way to have the public interface in a module only: Have build_target also be able to take opaque type objects that you can return from a module, i.e.
Right now, what I have in mind is something like
Which build directory? There are no per-target build directories except the private one, aren't there? This would have to be a per-target build directory since the bundle should effectively act like a single file, and that's how it already works right now. It should not contain "build residue" such as the private directory and whatnot, and you should be able to create more than one bundle target per project. Otherwise I don't see how this would be more practical to use than the current workaround in the documentation.
Yeah. This is working fine already though. Building a project with an app bundle target as of right now builds an app bundle in the same place an executable would be, which you can run from the build directory. |
The Ninja bundle builder uses this to merge a user supplied Info.plist with some Meson-generated data.
In case the source file is built by a target, the input file path needs to be the same as the target that generates the input file in order for Ninja to set up the target dependency.
7976bcc
to
3c49081
Compare
Adds macOS (, Swift, GNUstep, whatever else) bundle support.
Still WIP but it can build a simple bundle at this point.
TODO:
Maybe:
Don't plan on writing:
Closes #48. 🎉