Open Bug 1063329 Opened 10 years ago Updated 2 years ago

Add support for build-backend to generated Xcode projects

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
REOPENED

People

(Reporter: TYLin, Unassigned)

References

Details

Attachments

(3 files, 10 obsolete files)

part2-add-ancillary-files.diff
9.23 KB, patch
gps
: feedback+
Details | Diff | Splinter Review
part3-add-xcode_backend.py
14.88 KB, patch
gps
: feedback+
Details | Diff | Splinter Review
part1-add-option-to-backend.diff
5.05 KB, patch
gps
: feedback+
Details | Diff | Splinter Review
According to mach build-backend -h, we support the following target {RecursiveMake,AndroidEclipse,CppEclipse,VisualStudio}. It will be awesome to generate Xcode project to make developing on Mac OS easier.
This is likely low priority since Mac users can run Eclipse.
Blocks: Graphics_Backlog
This is what I have been using to edit Gecko in Xcode: https://github.com/garvankeeley/xcode-gen-firefox This is a throwaway prototype/proof-of-concept of how I would add this to build system. That is, generate xcconfig file(s) for build rules, and use the command-line util `mod-pbxproj` to add the list of source and header files to a stub pbxproj. If I find the time to understand the build backend system, I'll add an Xcode project generator using this method.
kip: perhaps you could help improve this? Seems relevant to your interests no matter what.
Flags: needinfo?(kgilbert)
(In reply to Nick Alexander :nalexander from comment #3) > kip: perhaps you could help improve this? Seems relevant to your interests > no matter what. I am an Xcode user, and would be glad to help out with this, once I catch up on my current bugs.
Flags: needinfo?(kgilbert)
Attached patch WIP mozbuild Xcode backend (obsolete) — — Splinter Review
A WIP patch. I documented some of my key misunderstandings of the build backend in the patch, and how I am working around my lack of understanding. This does generate an Xcode project in about 80s on my mac. The majority of files build. Various modules are not building (cairo, a number of 3rd party libs). It regenerates the Xcode project each time, but in future it should update in-place. I'll try to pick away at this patch over time, and whittle down the build errors.
Attachment #8573680 - Attachment description: xcode_bug1063329.diff → WIP mozbuild Xcode backend
I forgot to mention, build with `mach build-backend -b xcode`
I've heard stories about numerous large projects "outgrowing" Xcode. Essentially, parts of Xcode can't scale to handle very large source bases and performance grinds to a halt. I'm all for supporting whatever IDE people feel most productive in. But if Xcode performance is abysmal, we should consider leaving it out and redirecting people towards Eclipse, which AFAIK has better performance characteristics for very large projects. I'd appreciate if you could comment on the performance of the Xcode project while you are hacking on this.
I am not aware of the performance problems you are referring to. I have used it on big projects for about a decade. I have been using it with the Gecko code base lumped into a single project for many months, and I have seen no issues. However, I am using (and have always used) top-of-the-line mac hardware. I am trying to think of what might be of concern, perhaps: 1) generation of code-completion, but that is on a non-UI thread and doesn't seem to interfere with using the IDE 2) time to start compiling (built-in dependancy checking), which takes a few seconds, but nothing too severe. I have no evidence to support the next statement, but I think potential contributors on the mac are more likely to poke around in the code given an Xcode project is available. With regards to Eclipse support on the mac, I would be hesitant to point contributors to it, unless it gets completed. Last I checked it wasn't compiling, and had only a subset of the project code-completing.
(In reply to Gregory Szorc [:gps] from comment #7) > I've heard stories about numerous large projects "outgrowing" Xcode. > Essentially, parts of Xcode can't scale to handle very large source bases > and performance grinds to a halt. > > I'm all for supporting whatever IDE people feel most productive in. But if > Xcode performance is abysmal, we should consider leaving it out and > redirecting people towards Eclipse, which AFAIK has better performance > characteristics for very large projects. > > I'd appreciate if you could comment on the performance of the Xcode project > while you are hacking on this. Performance-wise, XCode is not the best. Once indexing has finished, it is workable though. The primary reasons I use XCode when developing for Apple platforms: - Integration with profiling tools on Apple platforms (can display dtrace counters in-line with source code) - Visually stepping through and inspecting stacks of multiple threads. - Audible breakpoints - Familiarity Often I find that I do edit in vim and compile from the command line, then switch to XCode when it comes to debugging, profiling, and tracing memory issues.
In consume_object(self, obj), if isinstance(obj, Sources) what is the build args (header search paths, preprocessor defines, flags) for this obj?
Flags: needinfo?(gps)
(In reply to Garvan Keeley [:garvank] from comment #10) > In consume_object(self, obj), > if isinstance(obj, Sources) what is the build args (header search paths, > preprocessor defines, flags) for this obj? We don't yet have full compiler flags available inside moz.build. However, most IDEs only need include search paths and preprocessor defines to work properly. These are available in the "Defines" and "LocalInclude" objects, respectively. You should look at visualstudio.py as an example.
Flags: needinfo?(gps)
I'll try take a look at VS one, two speed bumps for me with that 1) it is a big chunk of code 2) I am on a mac. I followed the Eclipse project generation, and got a similar rate of successful compilation (the Xcode one is higher, but I used an extremely inefficient method of setting the full set of build commands per-file -which was just an experiment to see how that would improve the rate compilation completion). The Xcode generator and the Eclipse one use Defines and LocalIncludes and yet a significant chunk of the project doesn't compile because of missing/improper defines (or other compiler flags missing, I haven't spent time looking at the failures, I'll do that when I come back to this). ASIDE: VS gen is setting up many (dozens?) of sub projects. I hope I don't have do the same method. Since I am not actually writing a project to produce a functional product, and getting Xcode to work with a similar subproject system is much more work, I prefer to have all the files in a single project, and each file compiling successfully.
Is there a strict order of object type arrival in consume_object()? That is, if I am consuming Source files, is there any guaranteed the Defines will arrive before the Source is consumed?
Flags: needinfo?(gps)
(In reply to Garvan Keeley [:garvank] from comment #13) > Is there a strict order of object type arrival in consume_object()? > > That is, if I am consuming Source files, is there any guaranteed the Defines > will arrive before the Source is consumed? Today, yes, the order is constant due to how the code works. https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/emitter.py#500 However, this is not a safe assumption to make for the future. You should buffer state and write only once all state becomes available. Alternatively, you should write unit tests that assert preconditions.
Flags: needinfo?(gps)
Ok thanks. My patch doesn't make order assumptions as it maintains a mapping of flags for the current directory.
Attached patch Xcode backend, WIP patch (obsolete) — — Splinter Review
Run with `mach build-backend -b xcode` Updated patch, only ~20 files not compiling. Here is a screenshot of the current list of files not compiling. Most are webrtc. https://www.dropbox.com/s/qoj6k248orfvny0/Screenshot%202015-04-01%2021.32.53.png
Attachment #8573680 - Attachment is obsolete: true
This builds ~1300 object files, whereas `mach build` builds ~2500 object files. The Xcode project does not compile any 3rd party sources or libs (the headers are included, just not the source files). I am ok with this behaviour, I think that 3rd party libs coding work would happen independent of the firefox codebase.
I use my hand-crafted xcode project. I only use it for code editing and debugging. not for compiling (so giving me an experience close to windows VisualStudio backend). That gives me auto-completion, search of code, browsing classes and all the xcode goodies. Not having the 3rd party libraries would be a major PITA as I personally work with those 3rd party libraries and we have our own code in some of them (like media/libstagefright/bindings). So they aren't really 3rd parties.
This is a great start ! :) Are you opened to suggestions / requests ? Would it be possible to have the file sorted alphabetically? makes it hard to find the file you want. Is it possible to keep the original files structure? rather than have all the files in the same unified block together? Could you make it so that all files are added, regardless of being used or not, but simply not a member of the final target? We have a lot of platform dependent code, and if the aim is to use XCode as an IDE, being able to see and modify files even if they aren't going to be built is still required (and still have them indexed to facilitate code browsing).
Regarding 3rd party libs: Thanks for the feedback, that is good to know that they are developed within the gecko codebase. I assume that I can add them without too much trouble, I will just have to read some more of the project generator code and see what class type is used to define a library -it probably has an obvious name like 'Library' :).
It seems that some files fail to compile as they include the wrong headers (typically a C++ headers). Like the VP9 encoder fails to compile as it attempts to include "string.h", but instead "String.h" (from js/src/vm/String.h) is used (my partition is the default HFS+ case insensitive) some -I magic probably needs to be added there by the look of things.
> Are you opened to suggestions / requests ? Definitely! I am doing it for the benefit of others -like you I have a working system external to our project generation so I don't need this, but it would be cool to have it. > Would it be possible to have the file sorted alphabetically? makes it hard > to find the file you want. Yup, I will add that. > Is it possible to keep the original files structure? rather than have all > the files in the same unified block together? I am confused: the generated project matches the structure on disk, no? Well, at least it does for the files I am working with. Can you screenshot this or provide more details where to look to see this problem? > Could you make it so that all files are added, regardless of being used or > not, but simply not a member of the final target? I have Xcode target control on a per-file basis. The project generation system only informs me of files that are part of the OS X platform. Obviously it is do-able, but I'll likely stop work on this patch I have the last few bugs ironed out.
(In reply to Jean-Yves Avenard [:jya] from comment #21) > It seems that some files fail to compile as they include the wrong headers > (typically a C++ headers). > Like the VP9 encoder fails to compile as it attempts to include "string.h", > but instead "String.h" (from js/src/vm/String.h) is used (my partition is > the default HFS+ case insensitive) > > some -I magic probably needs to be added there by the look of things. That is critical info thanks! I have some band-aid includes paths, because for reasons unknown, they aren't being passed in to the Xcode generator. I could see a header conflict happening because I am adding these paths globally. I need to solve why these paths are missing. The paths that I have to manually add in my patch are here: ['netwerk/sctp/src', 'ipc/chromium/src', + 'security/nss/lib/freebl/ecl', + 'intl/icu/source/common', + 'intl/icu/source/i18n']) <snip> +'dist/include') + ' ' + 'dist/include/cairo/') + ' ' + 'dist/nspr-include') + ' ') I notice the Eclipse project generator has to do the same (or very similar) manual path addition.
(In reply to Garvan Keeley [:garvank] from comment #22) > > Are you opened to suggestions / requests ? > > Definitely! I am doing it for the benefit of others -like you I have a > working system external to our project generation so I don't need this, but > it would be cool to have it. indeed. And looking at the compilation failure, I think they are actually genuine failures ; making me wonder how it built in the first place. > > Is it possible to keep the original files structure? rather than have all > > the files in the same unified block together? > > I am confused: the generated project matches the structure on disk, no? > Well, at least it does for the files I am working with. Can you screenshot > this or provide more details where to look to see this problem? if you check for example: dom/media/fmp4 you'll see that we have all flat file structure there. But it's made of a few sub-directories, one per platform. in media/libstagefright we have our own code in bindings, and the actual 3rd party lib in frameworks. Here media/libstagefright shows all files flat, regardless of their original structure. Now I understand this is eclipse does as well ; but I find it a pain to quickly find the file you want. something is still amiss, probably in the header search paths. in media/libstagefright/bindings/H264.cpp it includes media/stagefright/foundation/ABitReader.h which can't be found by xcode so the type: stagefright::ABitReader can't be resolved. > > > Could you make it so that all files are added, regardless of being used or > > not, but simply not a member of the final target? > > I have Xcode target control on a per-file basis. The project generation > system only informs me of files that are part of the OS X platform. > Obviously it is do-able, but I'll likely stop work on this patch I have the > last few bugs ironed out. I understand.. For this I will probably continue to use my hand-crafted xcode which was made by adding all files at once in the project and then playing with the "Header Search Paths" (I have HEADER_SEARCH_PATHS = /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/base /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/certdb /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/certhigh /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/ckfw /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/crmf /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/cryptohi /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/dbm /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/dev /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/freebl /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/jar /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/libpkix /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/Makefile /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/manifest.mn /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/nss /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/pk11wrap /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/pkcs12 /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/pkcs7 /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/pki /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/smime /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/softoken /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/sqlite /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/ssl /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/sysinit /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/util /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/zlib /Users/jyavenard/Work/Mozilla/mozilla-central/netwerk/sctp/src /Users/jyavenard/Work/Mozilla/mozilla-central/ipc/chromium/src /Users/jyavenard/Work/Mozilla/mozilla-central/security/nss/lib/freebl/ecl /Users/jyavenard/Work/Mozilla/mozilla-central/intl/icu/source/common /Users/jyavenard/Work/Mozilla/mozilla-central/intl/icu/source/i18n /Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/include /Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/include/cairo/ /Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/nspr-include I think having something like that by default would probably fix most of the compilation errors. like modules/libmar/nss_secutil.h getting dom/indexedDB/Key.h in place of "include/nss/key.h"
BTW, if you are curious, the build settings are mostly per-file: https://www.dropbox.com/s/trnqpbzkrz7u1a6/Screenshot%202015-04-07%2021.52.46.png?dl=0 Although the config.xconfig has a bunch of global include paths (HEADER_SEARCH_PATHS), which should be per-file instead, or per module. If I remove them, the rate of failed compilation goes way up.
(In reply to Jean-Yves Avenard [:jya] from comment #24) > (In reply to Garvan Keeley [:ga > if you check for example: dom/media/fmp4 you'll see that we have all flat > file structure there. > But it's made of a few sub-directories, one per platform. I see now. Weird. The files are passed into the project generator a little differently for these cases I assume. Will fix.
Aha, I forgot to shut off hmap generation (USE_HEADERMAP=NO), so Xcode was trying to be oh-so-helpful and find headers for me, resulting in conflicts such as the string.h one you mentioned.
Attachment #8587122 - Attachment is obsolete: true
I haven't included the libs yet, so it is still only building ~1200 files, but of these, 13 are not building, 9 of which are all media/webrtc/signaling/src/peerconnection :jya if you are familiar with this part of the code, and feel inclined to take a look, can you suggest what might be wrong. Your last feedback was quite helpful. I think a defines conflict around USE_FAKE_MEDIA_STREAMS is the root problem. TODO (in order): - folderize files in dom/media/fmp4 - fix duplicate .h files being added to project - add libs - bang head against desk because I am sure the last step just introduced more bugs
(In reply to Garvan Keeley [:garvank] from comment #29) > I haven't included the libs yet, so it is still only building ~1200 files, > but of these, 13 are not building, 9 of which are all > media/webrtc/signaling/src/peerconnection ^^ I have most of this fixed. > TODO (in order): > - folderize files in dom/media/fmp4 fixed > - fix duplicate .h files being added to project fixed need to rebase my patch on m-c head and post the latest WIP
Thanks for that... With the patch included it breaks non Mac compilation try compilation however (module not found). I inadvertently push a queue with this patch in.
Attached patch part1-add-option-to-backend.diff (obsolete) — — Splinter Review
Attachment #8590050 - Attachment is obsolete: true
Attached patch part2-add-ancillary-files.diff (obsolete) — — Splinter Review
Attached patch part3 - add xcode_backend.py (obsolete) — — Splinter Review
Now have ~1450 object files in project, only 4 not compiling. Unified_cpp_src_common0.cpp: missing define for N_UNDF WebrtcGlobalInformation.cpp: conflicting defines for internal/external linkage Unified_cpp_xpcom_build2.cpp (PoisonIOInterposerMac.cpp): seems to be missing unistd.h (missing lseek) storage/src/TelemetryVFS.cpp: No member named 'xFetch' in 'sqlite3_io_methods'
Attachment #8592348 - Attachment is patch: true
Attachment #8592348 - Attachment mime type: text/x-python-script → text/plain
Attached patch part2-add-ancillary-files.diff (obsolete) — — Splinter Review
Attachment #8592340 - Attachment is obsolete: true
Attached patch part3-add-xcode_backend.py (obsolete) — — Splinter Review
Now builds 1500 object files, no errors.
Attachment #8592348 - Flags: feedback?(vng)
Vic, don't worry too much about the logic, just a skim for pythonic impropriety would be helpful. I know, some of those functions take an atrocious number of arguments. Sigh.
Comment on attachment 8592348 [details] [diff] [review] part3 - add xcode_backend.py Review of attachment 8592348 [details] [diff] [review]: ----------------------------------------------------------------- Minor nits, but otherwise this seems ok in the context of the build system calling into the backend.
Comment on attachment 8592348 [details] [diff] [review] part3 - add xcode_backend.py Review of attachment 8592348 [details] [diff] [review]: ----------------------------------------------------------------- Minor nits, but otherwise this seems ok in the context of the build system calling into the backend. ::: python/mozbuild/mozbuild/backend/xcode_backend.py @@ +31,5 @@ > + if flags: > + self._per_dir_sources_and_flags[directory][file]['flags'] += ' ' + flags > + > + # look for a matching header file > + if file[-2:] == '.h': Use .endswith to avoid an out of bounds error. @@ +93,5 @@ > + self._topsrcdir = obj.topsrcdir > + if not self._topobjdir: > + self._topobjdir = obj.topobjdir > + > + if isinstance(obj, DirectoryTraversal): I really don't like this consume_object method. It seems like it should be using a visitor pattern, but I don't think you can realistically do this without making changes across the build system. @@ +146,5 @@ > + added_flags += ' '.join(value) + ' ' > + else: > + added_flags += defines_dict_to_string({key: value}) + ' ' > + > + added_flags = re.sub(r'\s\S+\.js|\s\S+\.manifest|\s\S+\.py|\s\S+\.cpp', ' ', added_flags) You should probably add a re.IGNORE_CASE option here. @@ +260,5 @@ > + compiler_flags += ' -I{0}/intl/icu/source/common -I{0}/intl/icu/source/i18n'.format(self._topsrcdir) + \ > + ' -I' + os.path.join(self._topobjdir, 'dist/include') > + > + # xcode treats double spaces as terminators when parsing flags > + compiler_flags = compiler_flags.replace(' -', ' -').replace('/ -', ' -') ' '.join(compiler_flags.split()) will replace all multiple spaces properly for you.
Attached patch part3-add-xcode_backend.py (obsolete) — — Splinter Review
Attachment #8592348 - Attachment is obsolete: true
Attachment #8594780 - Attachment is obsolete: true
Attachment #8592348 - Flags: feedback?(vng)
Attachment #8595059 - Flags: review?(gps)
Thanks for skimming this vng, addressed your points, > ' '.join(compiler_flags.split()) will replace all multiple spaces properly > for you. this is the only one I didn't do, as I only want to replace '<space><space>-' with a single space (double spaces between argument flags), but any other double spaces I will leave as-is (for instance -DFOO="A double space", leave the value untouched).
Attachment #8592339 - Flags: review?(gps)
Comment on attachment 8594779 [details] [diff] [review] part2-add-ancillary-files.diff This patch is: - the 3rd party mod-pbxproj python module - the empty project template for Xcode
Attachment #8594779 - Flags: review?(gps)
The headers appear to be missing in the generated xcode project from media/*. So doing ctrl-command-uparrow does nothing on those files and you can't resolve symbols. Example is in media/libstagefright Also, the unified files are added as-is. Is this intended? you have files such as Unified_cpp_media_libstagefrigth1.cpp. It seems that only the unified files are added to the firefox target. I haven't been able to run the result either.
(In reply to Jean-Yves Avenard [:jya] from comment #43) > The headers appear to be missing in the generated xcode project from > media/*. So doing ctrl-command-uparrow does nothing on those files and you > can't resolve symbols. Missing headers are annoying for that reason you mention. Perhaps these aren't reported to the generator. IIRC I saw the visual studio generator adding headers of its own accord, I can look at that. > Also, the unified files are added as-is. Is this intended? > you have files such as Unified_cpp_media_libstagefrigth1.cpp. Yes. They need to be project visible for compilation. Myself, I find it useful to know what files are part of this and which aren't. > It seems that only the unified files are added to the firefox target. I'm not sure what you mean (maybe a bug here?). Only a portion of the compilation units in the target are unified, if the non-unified cpp is to be compiled, it is added to the target. Here is the start of the target member list that I see (shows a mix of unified and solo): https://www.dropbox.com/s/69tq0or8b126tmk/Screenshot%202015-04-20%2022.19.09.png > I haven't been able to run the result either. Run? There is no run :). As the project generator output message states, this project is for editing, predictive-compilation errors, code completion, and navigation, and actual product build and run is still done with mach.
(In reply to Garvan Keeley [:garvank] from comment #44) > (In reply to Jean-Yves Avenard [:jya] from comment #43) > > The headers appear to be missing in the generated xcode project from > > media/*. So doing ctrl-command-uparrow does nothing on those files and you > > can't resolve symbols. > > Missing headers are annoying for that reason you mention. Perhaps these > aren't reported to the generator. IIRC I saw the visual studio generator > adding headers of its own accord, I can look at that. it seems to be all the headers outside dom/ well, I'm selfishly more interested in media/libstagefright as that's the stuff I'm currently working on :) Actually, I ended up being able to go to the header, but that jumps to a file not in a xcode project, you also can't see compilation error in the header as a result. > I'm not sure what you mean (maybe a bug here?). Only a portion of the > compilation units in the target are unified, if the non-unified cpp is to be > compiled, it is added to the target. > Here is the start of the target member list that I see (shows a mix of > unified and solo): I guess only the files that are actually going to be compiled are added to the target. I would have assumed that for incremental build, using unified will slow things down within xcode. As it will recompile all the files part of the unified stuff rather than just the modified one and its related object.. > Run? There is no run :). As the project generator output message states, > this project is for editing, predictive-compilation errors, code completion, > and navigation, and actual product build and run is still done with mach. I added a little shell script that is run within xcode which calls mach build binaries. This allows to complete the build, start the binary and more importantly, automatically attach the debugger to it. Without being able to start the binary from within xcode, makes it rather lose the main advantage IMHO: which is using xcode as a debugger. eclipse sucks in comparison (always IMHO of course)
> I guess only the files that are actually going to be compiled are added to > the target. > I would have assumed that for incremental build, using unified will slow > things down within xcode. As it will recompile all the files part of the > unified stuff rather than just the modified one and its related object.. A cpp that is part of a unified file should take a bit longer. However, I just tried triggering a few, I couldn't find a unified file that takes more than 2 seconds to compile when only one file is changed that it contains. > > Run? There is no run :). As the project generator output message states, > > this project is for editing, predictive-compilation errors, code completion, > > and navigation, and actual product build and run is still done with mach. > > I added a little shell script that is run within xcode which calls mach > build binaries. This allows to complete the build, start the binary and more > importantly, automatically attach the debugger to it. > > Without being able to start the binary from within xcode, makes it rather > lose the main advantage IMHO: which is using xcode as a debugger. eclipse > sucks in comparison (always IMHO of course) I can look at adding a executable target, it seems useful (and user-friendly). It isn't related to being able to debug though. One can attach, or specify the debug executable in XCode (hmm, the latter might be frustrating ATM, I would think this would get overwritten when the project generation is re-run). I just attach to debug. Because we have 2 processes, firefox and plugin-container, I am not sure how I would set up an automated executable target for both.
Comment on attachment 8595059 [details] [diff] [review] part3-add-xcode_backend.py Removing request for review on this, I'll try address the request for "Library" object parsing by taking a look at visual studio gen. I'll add a step to just add all headers in all subdirectories to the project, I see that visual studio gen does that too. I'll see if i can add pre-built debug targets for both 'firefox' and 'plugin container' processes. I have no plans to change the other two patches, those are just bootstrappy stuff that isn't particularly interesting or likely to change.
Attachment #8595059 - Flags: review?(gps)
Comment on attachment 8592339 [details] [diff] [review] part1-add-option-to-backend.diff Review of attachment 8592339 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/mach_commands.py @@ +12,5 @@ > import subprocess > import which > > from mozbuild.backend.cpp_eclipse import CppEclipseBackend > +from mozbuild.backend.xcode_backend import XcodeBackend Is this imported symbol actually used? Is the line above it still necessary? @@ +28,5 @@ > @CommandProvider > class MachCommands(MachCommandBase): > @Command('ide', category='devenv', > description='Generate a project and launch an IDE.') > @CommandArgument('ide', choices=['eclipse', 'visualstudio', 'androidstudio', 'intellij']) You'll need to update choices to allow "xcode". While you're here, please also rename the method from "eclipse" to "ide".
Attachment #8592339 - Flags: review?(gps) → feedback+
Comment on attachment 8594779 [details] [diff] [review] part2-add-ancillary-files.diff Review of attachment 8594779 [details] [diff] [review]: ----------------------------------------------------------------- First, in theory we shouldn't need to check this into source control: mach commands can install packages into the virtualenv via pip. Since xcode support will initially be experimental, I'm inclined to take this approach instead of checking this code into the tree. If we do check this code in, it should be checked in under python/mod_pbxproj, a path should be added to build/virtualenv_packages.txt and/or build/mach_bootstrap.py. Also, the commit message should contain details about where this code was fetched from.
Attachment #8594779 - Flags: review?(gps)
Thanks for checking this out, > > from mozbuild.backend.cpp_eclipse import CppEclipseBackend > > +from mozbuild.backend.xcode_backend import XcodeBackend > > Is this imported symbol actually used? Is the line above it still necessary? Not needed. > @@ +28,5 @@ > > @CommandProvider > > class MachCommands(MachCommandBase): > > @Command('ide', category='devenv', > > description='Generate a project and launch an IDE.') > > @CommandArgument('ide', choices=['eclipse', 'visualstudio', 'androidstudio', 'intellij']) > > You'll need to update choices to allow "xcode". While you're here, please > also rename the method from "eclipse" to "ide". Will do. > First, in theory we shouldn't need to check this into source control: mach > commands can install packages into the virtualenv via pip. That is great, I am not a fan of checking it in. I'll take a look at that.
I was looking at the VS generator again, and it looks close/maybe/possibly to useable as a base for Xcode generation. The main problem I see is that it misses a ton of build flags. Critically, these defines are rarely being set: -DSTATIC_EXPORTABLE_JS_API -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL For instance, MOZILLA_INTERNAL_API is found only in these: library_dom_media.vcxproj library_dombindings_test_s.vcxproj library_ecc.vcxproj library_media_standalone.vcxproj library_mtransport_standalone.vcxproj library_necko_standalone.vcxproj library_StaticXULComponentsEnd.vcxproj library_unicharutil_standalone.vcxproj library_xpcomrt.vcxproj (On a mac build, this is defined in 730 of the 840 backend.mk) GPS, any idea why the difference? Or more critically, if there is an easy way tweak to VS generation to get these picked up.
Flags: needinfo?(gps)
Attached patch part1-add-option-to-backend.diff (obsolete) — — Splinter Review
Updated as per feedback, adds xcode option to mach_commands.py and config_status.py,
Attachment #8592339 - Attachment is obsolete: true
Attachment #8602300 - Flags: review?(gps)
Adds xcode templates. Removed mod-pbxproj, this is now pip installable. Problem: I don't see a requirements.txt that I can add mod-pbxproj.
Attachment #8602301 - Flags: review?(gps)
Attachment #8594779 - Attachment is obsolete: true
Updated: - grab all the .h files in the source tree to the project. - creates a default executable that can be debugged with ctrl-cmd-R in Xcode
Attachment #8595059 - Attachment is obsolete: true
Attachment #8602302 - Flags: review?(gps)
Attachment #8602302 - Attachment is patch: true
Attachment #8602302 - Attachment mime type: text/x-python-script → text/plain
Changes mentioned in comment 52, plus added step to pip install mod-pbxproj
Attachment #8602300 - Attachment is obsolete: true
Attachment #8602300 - Flags: review?(gps)
Attachment #8602339 - Flags: review?(gps)
Dear readers, ignore comment 55, manual pip install no longer needed.
Comment on attachment 8602339 [details] [diff] [review] part1-add-option-to-backend.diff Review of attachment 8602339 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/mach_commands.py @@ +32,3 @@ > @CommandArgument('args', nargs=argparse.REMAINDER) > + def ide(self, ide, args): > + if ide == 'xcode': The list of comparisons should ideally be alphabetized. And, you should be using elif, not two ifs. ::: python/mozbuild/mozbuild/config_status.py @@ +128,5 @@ > elif options.backend == 'VisualStudio': > from mozbuild.backend.visualstudio import VisualStudioBackend > backend_cls = VisualStudioBackend > + elif options.backend == 'xcode': > + from mozbuild.backend.xcode_backend import XcodeBackend It feels weird to see the import of a file that hasn't been added yet. Perhaps this patch should be combined with another one?
Attachment #8602339 - Flags: review?(gps) → feedback+
Comment on attachment 8602301 [details] [diff] [review] part2-add-ancillary-files.diff Review of attachment 8602301 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/templates/xcode/stub_project.pbxproj @@ +6,5 @@ > + objectVersion = 46; > + objects = { > + > +/* Begin PBXFileReference section */ > + EB1183D81A75F5080094CA96 /* config.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = config.xcconfig; sourceTree = "<group>"; }; The magic values in this file are a bit scary. Could we get some inline docs or a link to docs explaining how this works? (Maybe something is in part 3?)
Attachment #8602301 - Flags: review?(gps) → feedback+
Comment on attachment 8602302 [details] [diff] [review] part3-add-xcode_backend.py Review of attachment 8602302 [details] [diff] [review]: ----------------------------------------------------------------- Lots of people have opinions on the IDE code and are willing to scratch itches. For this reason, I'd love to see more docstrings and comments about how all this works. Please add documentation to this code to enable others to more easily improve it. ::: python/mozbuild/mozbuild/backend/xcode_backend.py @@ +8,5 @@ > +import re > +from .common import CommonBackend > +from ..frontend.data import ( > + Defines, Exports, Sources, GeneratedSources, GeneratedInclude, UnifiedSources, LocalInclude, HostSources, > + DirectoryTraversal, PerSourceFlag, VariablePassthru, StaticLibrary) Nit, please alphabetize and split these out 1 per line. @@ +24,5 @@ > + self._topsrcdir = self.environment.topsrcdir > + self._moz_app_version = None > + self._xcode_groups = {} > + > + CommonBackend._init(self) You probably want this first. Otherwise, it could overwrite things above. @@ +26,5 @@ > + self._xcode_groups = {} > + > + CommonBackend._init(self) > + > + self._get_all_headers(self._topsrcdir) Do you have to call this here, or can it be deferred until consume_finished? I ask because this is an expensive operation (it walks the filesystem). It would be unfortunate if we did all this work then found out that the state of moz.build is invalid and encountered an error there. Deferring it until after moz.build processing results in less work when other parts are invalid. The general principle here is "fail fast:" do the flaky operations first and the robust operations later. @@ +32,5 @@ > + xcode_proj_path = os.path.join(self._topobjdir, 'firefox.xcodeproj/project.pbxproj') > + try: > + os.mkdir(os.path.dirname(xcode_proj_path)) > + except OSError: > + pass if e.errno != errno.EEXIST: raise Otherwise this ignores permissions failures. @@ +41,5 @@ > + xcode_proj_path) > + > + xcshared_file = os.path.dirname(xcode_proj_path) + "/xcshareddata/xcschemes/firefox.xcscheme" > + if not os.path.exists(os.path.dirname(xcshared_file)): > + os.makedirs(os.path.dirname(xcshared_file)) This is an anti-pattern. Call mkdir or makedirs unconditionally and ignore failures due to errno.EEXIST. @@ +47,5 @@ > + xcshared_file) > + with open(xcshared_file) as in_file: > + text = in_file.read() > + with open(xcshared_file, 'w') as out_file: > + out_file.write(text.replace('REPLACE_ME_WITH_PATH', self._topobjdir)) Replacement text should probably be TOPOBJDIR. @@ +72,5 @@ > + if not module_dir: > + return > + > + if not self._moz_app_version: > + self._moz_app_version = 'MOZ_APP_VERSION=\\"' + obj.config.substs["MOZ_APP_VERSION"] + '\\"' Do you know about Python's formatting operator? String concatenation almost never occurs in Python. Instead, you do something like: self._moz_app_version = 'MOZ_APP_VERION=\\"%s\\"' % obj.config.substs['MOZ_APP_VERSION'] @@ +77,5 @@ > + > + if isinstance(obj, DirectoryTraversal): > + self._add_per_relobjdir_build_args(obj.relobjdir, {'-I' + obj.srcdir}) > + > + objinc = self._topobjdir + "/" + module_dir Use os.path.join @@ +96,5 @@ > + build_args = parse_prefix_and_defines(obj.config.substs['OS_COMPILE_CFLAGS'], obj.topobjdir) > + self._add_per_relobjdir_build_args(obj.relobjdir, build_args) > + > + # todo: mystery as to why this is missing > + if 'gfx' in module_dir: wat? Is this absolutely needed? One-offs like this are evil. At the minimum, a follow-up bug should be filed and this should land with an in-line comment referencing that bug. @@ +101,5 @@ > + self._add_per_relobjdir_build_args(obj.relobjdir, > + {'-I' + './dist/include/cairo' + > + ' -I' + './gfx/cairo/cairo/src'}) > + > + if isinstance(obj, Exports): elif @@ +104,5 @@ > + > + if isinstance(obj, Exports): > + for path, files in obj.exports.walk(): > + if not files: > + continue Are these 2 lines needed? Won't files be an empty list? @@ +120,5 @@ > + if val is 'True': > + return '1' > + if val is 'False': > + return '0' > + return "'" + val + "'" This smells like something we've already written somewhere. While it may be a DRY violation, it's easy enough to grok. @@ +134,5 @@ > + > + pattern = re.compile(r'\S+\.js|\S+\.manifest|\S+\.py|\S+\.cpp', re.IGNORECASE) > + added_flags = {x for x in added_flags if not re.match(pattern, x)} > + if '-std=c99' in added_flags: > + added_flags.remove('-std=c99') Why? Please document. @@ +162,5 @@ > + self._topobjdir, obj.relobjdir, is_built=True) > + > + # todo looking at the preprocessed output, unistd is present, however, > + # without this mystery include, compilation fails > + if 'Unified_cpp_xpcom_build2' in unified_file: wat? Again, this should at least be a filed follow-up bug. Also, if the set of files going into this unified file change, this solution will stop working. @@ +184,5 @@ > + def consume_finished(self): > + self._flush_to_xcode() > + > + # For reasons unknown, various includes are missing. Adding them manually. > + import glob Please move this import to top of file. @@ +185,5 @@ > + self._flush_to_xcode() > + > + # For reasons unknown, various includes are missing. Adding them manually. > + import glob > + missing_topsrc_includes = glob.glob(os.path.join(self._topsrcdir, 'security/nss/lib') + "/*") Better yet, glob is somewhat evil. Can't you simply iterate over os.listdir here? @@ +191,5 @@ > + missing_topsrc_includes = [x for x in missing_topsrc_includes if 'sqlite' not in x] > + missing_topsrc_includes.extend(['netwerk/sctp/src', > + 'ipc/chromium/src', > + 'xpcom/tests', > + 'security/nss/lib/freebl/ecl']) These should be a file-level list, documented of course. @@ +194,5 @@ > + 'xpcom/tests', > + 'security/nss/lib/freebl/ecl']) > + output_xcconfig = os.path.join(self._topobjdir, 'config.xcconfig') > + shutil.copyfile(os.path.join(self._current_path, 'templates/xcode/stub_xcconfig'), output_xcconfig) > + f = open(output_xcconfig, 'a') Always use the context manager form of open(): with open(output_xconfig, 'a') as f: f.write(...) This way, file descriptors don't leak in case of error. (Not that it matters for this code, but best practices are best practices.) @@ +205,5 @@ > + f.close() > + > + self._xcode_project.save(sort=True) > + > + print 'Xcode file is at: ' + self._xcode_project.pbxproj_path print() @@ +210,5 @@ > + > + def _add_per_dir_source_and_flags(self, directory, file, abs_src_path, relobjdir, is_built=True, flags=''): > + # some files can arrive as a full path to file > + if abs_src_path in file: > + file = file.replace(abs_src_path + '/', '').replace(directory + '/', '') Manual path munging is not robust. Please use os.path.relpath. You may also find the mozpack.mozpath module useful. @@ +213,5 @@ > + if abs_src_path in file: > + file = file.replace(abs_src_path + '/', '').replace(directory + '/', '') > + > + if directory not in self._per_dir_sources_and_flags: > + self._per_dir_sources_and_flags[directory] = {} There are better ways to write this pattern: This is good: self._per_dir_sources_and_flags.setdefault(directory, {}) Even better is: self._per_dir_sources_and_flags = collections.defaultdit(dict) ... Then, missing key lookups automagically result in key creation :) @@ +216,5 @@ > + if directory not in self._per_dir_sources_and_flags: > + self._per_dir_sources_and_flags[directory] = {} > + if file not in self._per_dir_sources_and_flags[directory]: > + self._per_dir_sources_and_flags[directory][file] = {'abs_src_path': abs_src_path, 'is_built': is_built, > + 'flags': flags, 'relobjdir': relobjdir} This operation is idempotent, right? Perhaps it would just be easier to blindly assign? @@ +219,5 @@ > + self._per_dir_sources_and_flags[directory][file] = {'abs_src_path': abs_src_path, 'is_built': is_built, > + 'flags': flags, 'relobjdir': relobjdir} > + > + if flags: > + self._per_dir_sources_and_flags[directory][file]['flags'] += ' ' + flags It /might/ be better to store flags as a list and convert to string at the last moment. Generally speaking, anywhere I see string concatenation in Python that isn't related to final output sets off alarm bells. @@ +226,5 @@ > + return > + > + # look for a matching header file > + header_file = file.rsplit(".", 1)[0] + ".h" > + for src_path in (self._topsrcdir, self._topobjdir): I'm not sure if you put topsrcdir first intentionally. But it should be first because we want to prioritize versions in the source directory to facilitate editing (assuming content is equivalent, anyway). That should be explicitly documented. The content comparison can likely be deferred to a follow-up. But it should also be documented. FWIW, the Visual Studio files IIRC take the approach of not listing headers because of ambiguities. If the project knows how to build, it can presumably find the correct header. I'd rather trust the compiler's knowledge than trying to implement header lookup here. @@ +227,5 @@ > + > + # look for a matching header file > + header_file = file.rsplit(".", 1)[0] + ".h" > + for src_path in (self._topsrcdir, self._topobjdir): > + if os.path.exists(os.path.join(src_path, directory + '/' + header_file)): You are already using os.path.join. Just pass 3 arguments instead of putting the '/' in here? @@ +231,5 @@ > + if os.path.exists(os.path.join(src_path, directory + '/' + header_file)): > + self._add_per_dir_source_and_flags(directory, header_file, src_path, relobjdir) > + break > + > + def _add_per_relobjdir_build_args(self, directory, set_of_build_args): "set_of_" can be dropped. @@ +233,5 @@ > + break > + > + def _add_per_relobjdir_build_args(self, directory, set_of_build_args): > + if directory not in self._per_dir_defines_includes_and_flags: > + self._per_dir_defines_includes_and_flags[directory] = list(set_of_build_args) Use defaulftdict @@ +237,5 @@ > + self._per_dir_defines_includes_and_flags[directory] = list(set_of_build_args) > + else: > + existing = self._per_dir_defines_includes_and_flags[directory] > + args = [x for x in set_of_build_args if x not in existing] > + self._per_dir_defines_includes_and_flags[directory].extend(args) It looks like you want a set ordered by insert order? You can use collections.OrderedDict for this purpose. Just ignore the values. (We should ideally have an OrderedSet in mozbuild.util.) @@ +240,5 @@ > + args = [x for x in set_of_build_args if x not in existing] > + self._per_dir_defines_includes_and_flags[directory].extend(args) > + > + def _get_or_create_xcode_group(self, abs_src_path, sub_path, depth=0): > + if not sub_path or len(sub_path) < 2: Magic number 2 definitely needs some comments. @@ +246,5 @@ > + > + path = os.path.join(abs_src_path, sub_path) > + > + if path in self._xcode_groups: > + return self._xcode_groups[path] It probably doesn't matter here, but it is common to use .get to avoid double key lookups: group = self._xcode_groups.get(path) if group: return group This pattern can save you when dicts are big and/or you are in a tight loop and performance matters. .get will return None by default. You can pass a 2nd argument to define what will be returned if the key doesn't exist. @@ +269,5 @@ > + def _add_file_to_xcode_group(self, group, full_path_to_file, is_built, flags): > + result = self._xcode_project.add_file(full_path_to_file, group, > + create_build_files=is_built, ignore_unknown_type=True) > + if len(result) == 0: > + return This is a Python anti-pattern: `if foo` is False for empty container types like lists, dicts, and sets. @@ +298,5 @@ > + for root, dirs, files in os.walk(directory): > + dirs[:] = [d for d in dirs if not d.startswith('.')] > + for file in files: > + if file.endswith('.h'): > + module = root.replace(directory + '/', '') .replace is the wrong manipulation here. You want to slice off the first N characters. But, there is a better way: os.path.relpath() does exactly this in a path-aware manner.
Attachment #8602302 - Flags: review?(gps) → feedback+
I suspect the issue behind the needinfo has been resolved.
Flags: needinfo?(gps)
Ehsan pointed me to 904572, so I can dump the entire approach in xcode_backend.py (which is too many loc, and too fragile), and wrap the output from that patch (hopefully in a very concise fashion).
Depends on: 904572
> ::: python/mozbuild/mozbuild/backend/templates/xcode/stub_project.pbxproj > @@ +6,5 @@ > > + objectVersion = 46; > > + objects = { > > + > > +/* Begin PBXFileReference section */ > > + EB1183D81A75F5080094CA96 /* config.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = config.xcconfig; sourceTree = "<group>"; }; > > The magic values in this file are a bit scary. Could we get some inline docs > or a link to docs explaining how this works? (Maybe something is in part 3?) Yeah, it is ugly. This stub is autogenerated from Xcode, which uses GUIDs in its file format. Each GUID is defined elsewhere in the file. To generate this, I create a new project in Xcode and setup a few basic items (project name, project type). In future I could look at generating this from scratch.
With the advent of the (awesome) CompileDB option on mach, there is no reason I see to have this in-tree: 1) there is enough complexity in the build system 2) it is cleaner to have this as a separate entity 3) it is a fringe feature of the build system (only ~5 people have pinged me about this) Here is a github project that WFM: https://github.com/garvankeeley/gecko-xcode-proj-generator
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System

i believe there could be renewed interest in this with the increase in need for multi-process debugging and in particular the fission project.

So far, in the supported backend, only Visual Studio C++ (not Code) has the ability to be configured to automatically attach to a new process.

Visual Studio Code can only let you debug a single process at a time (you can do more, but it's so painful to setup it's not worth it).

XCode has the ability to debug all processes setup in an application (Scheme -> Edit Scheme -> Options -> Debug XPC services used by this application), so having the ability to generate an xcode project would make help greatly.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: