Bug 182891 – [CMake] Replace AVFoundationSupport.py using CMake

• Component: CMake
• Status: RESOLVED FIXED

[Don Olmstead]

Currently AVFoundationSupport.py looks around for include files. This is something that CMake could be used for instead to determine.

[Don Olmstead]

Created attachment 334394 [details]
Patch

This is going to need to be tested on the internal AppleWin build.

Please run it initially to get the contents of WebKitBuild/Release/DerivedSources/PAL/PALHeaderDetection.h

From there make sure that the HAVE checks in cmakeconfig.h match up. If they do then this is good to go. If not then we need to start debugging in the CMake logs.

[Don Olmstead]

Created attachment 334423 [details]
Patch

Rebased on latest revision

[Don Olmstead]

Created attachment 334424 [details]
Patch

[Don Olmstead]

Comment on attachment 334424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334424&action=review

Ok this should hopefully work. Still needs to be checked on Apple Win internal builds

> Source/WebCore/config.h:-28
> -#if PLATFORM(COCOA)

This is already in Platform.h so no need to redefine it here.

[Don Olmstead]

Created attachment 334661 [details]
Patch

Rebasing after r229048

[EWS Watchlist]

Attachment 334661 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:62:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 334662 [details]
Patch

Make style queue happier...

[EWS Watchlist]

Attachment 334662 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:62:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 334750 [details]
Patch

Potential fix for apple internal build

[EWS Watchlist]

Attachment 334750 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:62:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 334759 [details]
Patch

Use HAVE_SYMBOL to hopefully get further in the apple internal build

[EWS Watchlist]

Attachment 334759 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:62:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/cmake/OptionsAppleWin.cmake:33:  No trailing spaces  [whitespace/trailing] [5]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Comment on attachment 334759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334759&action=review

> Source/cmake/OptionsAppleWin.cmake:43
> +    set(CMAKE_REQUIRED_INCLUDES ${WEBKIT_LIBRARIES_INCLUDE_DIR})
> +    set(CMAKE_REQUIRED_LIBRARIES
> +        "${WEBKIT_LIBRARIES_LINK_DIR}/CoreFoundation${DEBUG_SUFFIX}.lib"
> +        "${WEBKIT_LIBRARIES_LINK_DIR}/AVFoundationCF${DEBUG_SUFFIX}.lib"
> +        "${WEBKIT_LIBRARIES_LINK_DIR}/QuartzCore${DEBUG_SUFFIX}.lib"
> +    )
> +    
> +    WEBKIT_CHECK_HAVE_INCLUDE(HAVE_AVCF AVFoundationCF/AVCFBase.h)
> +
> +    if (HAVE_AVCF)
> +        SET_AND_EXPOSE_TO_BUILD(USE_AVFOUNDATION ON)
> +    endif ()
> +
> +    WEBKIT_CHECK_HAVE_SYMBOL(HAVE_AVCF_LEGIBLE_OUTPUT AVCFPlayerItemLegibleOutputSetCallbacks "TargetConditionals.h;dispatch/dispatch.h;AVFoundationCF/AVFoundationCF.h;AVFoundationCF/AVCFPlayerItemLegibleOutput.h")
> +    WEBKIT_CHECK_HAVE_SYMBOL(HAVE_AVFOUNDATION_LOADER_DELEGATE AVCFAssetResourceLoaderSetCallbacks "TargetConditionals.h;dispatch/dispatch.h;AVFoundationCF/AVFoundationCF.h")
> +    WEBKIT_CHECK_HAVE_SYMBOL(HAVE_AVCFURL_PLAYABLE_MIMETYPE AVCFURLAssetIsPlayableExtendedMIMEType "TargetConditionals.h;dispatch/dispatch.h;AVFoundationCF/AVFoundationCF.h")
> +    WEBKIT_CHECK_HAVE_SYMBOL(HAVE_AVCFPLAYERITEM_CALLBACK_VERSION_2 kAVCFPlayerItemLegibleOutput_CallbacksVersion_2 "TargetConditionals.h;dispatch/dispatch.h;AVFoundationCF/AVFoundationCF.h")

On public apple win I am using have symbol rather than looking directly for the include. The have symbol actually creates a C file and tries to compile and link it. The ones expected for public apple win are present so things SHOULD work for the internal apple build

[Don Olmstead]

Per Arne if everything is getting detected properly on Apple's internal windows build I'd like to get this reviewed and merged.

[Don Olmstead]

Created attachment 348206 [details]
Patch

Rebased and trying to use a compile check for the callback v2

[Don Olmstead]

Created attachment 348207 [details]
Patch

[EWS Watchlist]

Attachment 348207 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:57:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Comment on attachment 348207 [details]
Patch

Per Arne everything seems to have built on AppleWin side. Just need confirmation that AppleWin internal builds are happy and have the right values present. If so its good to go.

[Don Olmstead]

Created attachment 348325 [details]
Patch

Verified that the public AppleWin build has the same behavior through the CMake checks.

https://gist.github.com/donny-dont/78a553447df58882b38854863749a6d1 has the outputted cmakeconfig.h

[Don Olmstead]

Comment on attachment 348325 [details]
Patch

Still needs to be verified with the internal AppleWin builds to make sure the have checks pick up everything properly.

[EWS Watchlist]

Attachment 348325 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:40:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:57:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Per Arne Vollan]

Comment on attachment 348325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348325&action=review

> Source/WebCore/config.h:-30
> -#if PLATFORM(COCOA)
> -#define USE_FILE_LOCK 1
> -#endif

Is it correct to remove this define?

[Don Olmstead]

Created attachment 348355 [details]
Patch

Updating based on review feedback

[EWS Watchlist]

Attachment 348355 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:55:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 348359 [details]
Patch

[EWS Watchlist]

Comment on attachment 348359 [details]
Patch

Attachment 348359 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9018378

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 348381 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Don Olmstead]

(In reply to Build Bot from comment #27)
> Created attachment 348381 [details]
> Archive of layout-test-results from ews106 for mac-sierra-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

I don't see why this would be caused by this patch and I saw a couple bot failures on mac-wk2 for other patches.

Per Arne, Alex or Brent could you give this a review? This should work for Apple's internal builds. I tested it as much as I could without access to it.

[Per Arne Vollan]

Comment on attachment 348359 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348359&action=review

r=me. Please make sure the bots are green before landing.

> Source/WebCore/PAL/ChangeLog:8
> +        Remove AVFoundationSupport.py file and invokation.

invocation

> Source/WebKitLegacy/win/WebKitPrefix.h:69
> +#if USE(CG)
> +#ifndef CGFLOAT_DEFINED
> +#if (defined(__LP64__) && __LP64__) || (defined(__x86_64__) && __x86_64__) || defined(_M_X64) || defined(__amd64__)
> +typedef double CGFloat;
> +#else
> +typedef float CGFloat;
> +#endif
> +#define CGFLOAT_DEFINED 1
> +#endif
> +#endif /* USE(CG) */

This could perhaps be put in a new header file (CGFloat.h) to avoid duplication.

> Source/cmake/WebKitCommon.cmake:-78
> -        file(MAKE_DIRECTORY ${DERIVED_SOURCES_PAL_DIR})

Is creating this directory not needed anymore?

[Don Olmstead]

Created attachment 348532 [details]
Patch

Updated based on review

[Don Olmstead]

Created attachment 348544 [details]
Patch

Ok it appears those #defines are needed in config.h so adding them back

[Don Olmstead]

(In reply to Per Arne Vollan from comment #29)
> Comment on attachment 348359 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348359&action=review
> 
> r=me. Please make sure the bots are green before landing.
> 
> > Source/WebCore/PAL/ChangeLog:8
> > +        Remove AVFoundationSupport.py file and invokation.
> 
> invocation
> 
> > Source/WebKitLegacy/win/WebKitPrefix.h:69
> > +#if USE(CG)
> > +#ifndef CGFLOAT_DEFINED
> > +#if (defined(__LP64__) && __LP64__) || (defined(__x86_64__) && __x86_64__) || defined(_M_X64) || defined(__amd64__)
> > +typedef double CGFloat;
> > +#else
> > +typedef float CGFloat;
> > +#endif
> > +#define CGFLOAT_DEFINED 1
> > +#endif
> > +#endif /* USE(CG) */
> 
> This could perhaps be put in a new header file (CGFloat.h) to avoid
> duplication.

Will look into getting rid of this in a follow up.

> 
> > Source/cmake/WebKitCommon.cmake:-78
> > -        file(MAKE_DIRECTORY ${DERIVED_SOURCES_PAL_DIR})
> 
> Is creating this directory not needed anymore?

Nope its not. AVFoundationSupport.py was the only thing writing to it.

[WebKit Commit Bot]

Comment on attachment 348544 [details]
Patch

Clearing flags on attachment: 348544

Committed r235531: <https://trac.webkit.org/changeset/235531>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

*** Bug 182800 has been marked as a duplicate of this bug. ***

[Ross Kirsling]

Committed build fix for WPE in r235536: <https://trac.webkit.org/changeset/235536>