180949 2017-12-18 14:11:55 -0800 [GTK][WPE] Conditionalize libTASN1 use behind ENABLE_SUBTLE_CRYPTO in the CMake files 2017-12-19 16:47:51 -0800 1 1 1 Unclassified WebKit WebCore Misc. Other Other Linux RESOLVED WONTFIX InRadar P2 Normal --- 181012 1 timothy timothy cgarcia clopez commit-queue mcatanzaro webkit-bug-importer oldest_to_newest 1382902 0 timothy 2017-12-18 14:11:55 -0800 When ENABLE_SUBTLE_CRYPTO is disable the include paths and libraries should not include missing LIBTASN1 references. 1382904 1 329682 timothy 2017-12-18 14:13:53 -0800 Created attachment 329682 Patch 1383038 2 329682 commit-queue 2017-12-18 20:09:14 -0800 Comment on attachment 329682 Patch Clearing flags on attachment: 329682 Committed r226094: <https://trac.webkit.org/changeset/226094> 1383039 3 commit-queue 2017-12-18 20:09:16 -0800 All reviewed patches have been landed. Closing bug. 1383040 4 webkit-bug-importer 2017-12-18 20:10:17 -0800 <rdar://problem/36123388> 1383147 5 mcatanzaro 2017-12-19 09:10:31 -0800 This actually is not the best way to do it. Two problems: 1) The check is still done unconditionally in OptionsWPE.cmake, even though ENABLE_SUBTLE_CRYPTO is no longer mandatory for WPE. That's wrong. So it's currently impossible to build without Libtasn1, and the added conditionals in PlatformWPE.cmake are useless as they'll never be reached. The check for Libtasn1 should instead be moved down and placed inside an if (ENABLE_SUBTLE_CRYPTO) block in OptionsWPE.cmake, then it will really be possible to build without. 2) It's simpler to unconditionally add _LIBRARIES and _INCLUDE_DIRS in Platform*.cmake. If the checks are made conditional in Options*.cmake, then (I think) the _LIBRARIES and _INCLUDE_DIRS variables will expand to empty strings, so adding conditionals in Platform*.cmake should not be needed. 1383148 6 mcatanzaro 2017-12-19 09:16:56 -0800 (In reply to Michael Catanzaro from comment #5) > 2) It's simpler to unconditionally add _LIBRARIES and _INCLUDE_DIRS in > Platform*.cmake. If the checks are made conditional in Options*.cmake, then > (I think) the _LIBRARIES and _INCLUDE_DIRS variables will expand to empty > strings, so adding conditionals in Platform*.cmake should not be needed. Although it might be a bit confusing to intentionally use undefined variables, it is IMO good style in this case, because the Platform*.cmake files will otherwise become a mess of unneeded conditionals for different build configurations. It's unfortunate that the existing code does not always follow this style. E.g.: # Source/WebCore/PlatformGTK.cmake if (ENABLE_WAYLAND_TARGET) list(APPEND WebCore_SYSTEM_INCLUDE_DIRECTORIES ${WAYLAND_INCLUDE_DIRS} ) list(APPEND WebCore_LIBRARIES ${WAYLAND_LIBRARIES} ) endif () I believe that condition is not needed, because ${WAYLAND_INCLUDE_DIRS} and ${WAYLAND_LIBRARIES} are only defined if ENABLE_WAYLAND_TARGET is true; they can be appended to WebCore_SYSTEM_INCLUDE_DIRECTORIES and WebCore_LIBRARIES unconditionally. 1383152 7 timothy 2017-12-19 09:21:23 -0800 (In reply to Michael Catanzaro from comment #6) > (In reply to Michael Catanzaro from comment #5) > > 2) It's simpler to unconditionally add _LIBRARIES and _INCLUDE_DIRS in > > Platform*.cmake. If the checks are made conditional in Options*.cmake, then > > (I think) the _LIBRARIES and _INCLUDE_DIRS variables will expand to empty > > strings, so adding conditionals in Platform*.cmake should not be needed. > > Although it might be a bit confusing to intentionally use undefined > variables, it is IMO good style in this case, because the Platform*.cmake > files will otherwise become a mess of unneeded conditionals for different > build configurations. > > It's unfortunate that the existing code does not always follow this style. > E.g.: > > # Source/WebCore/PlatformGTK.cmake > if (ENABLE_WAYLAND_TARGET) > list(APPEND WebCore_SYSTEM_INCLUDE_DIRECTORIES > ${WAYLAND_INCLUDE_DIRS} > ) > list(APPEND WebCore_LIBRARIES > ${WAYLAND_LIBRARIES} > ) > endif () > > I believe that condition is not needed, because ${WAYLAND_INCLUDE_DIRS} and > ${WAYLAND_LIBRARIES} are only defined if ENABLE_WAYLAND_TARGET is true; they > can be appended to WebCore_SYSTEM_INCLUDE_DIRECTORIES and WebCore_LIBRARIES > unconditionally. I was hitting a linker or build error (can't recall which, this was a while ago) when they were unconditional. I wouldn't have touched this otherwise. Maybe just a side effect of our platform at Tesla? I was just following the style of similar conditionals. If you feel this should be cleaned up for all cases, then let track that separately. I don't have the cycles to do it though. 1383156 8 mcatanzaro 2017-12-19 09:25:28 -0800 (In reply to Timothy Hatcher from comment #7) > I was hitting a linker or build error (can't recall which, this was a while > ago) when they were unconditional. I wouldn't have touched this otherwise. > Maybe just a side effect of our platform at Tesla? I don't know. I don't expect you to clean up all cases, just Libtasn1, since that's what this patch is about. I would revert this patch and change the unconditional check for Libtasn1 in OptionsWPE.cmake to look more like the check in OptionsGTK.cmake instead. 1383157 9 timothy 2017-12-19 09:29:39 -0800 (In reply to Michael Catanzaro from comment #8) > (In reply to Timothy Hatcher from comment #7) > > I was hitting a linker or build error (can't recall which, this was a while > > ago) when they were unconditional. I wouldn't have touched this otherwise. > > Maybe just a side effect of our platform at Tesla? > > I don't know. > > I don't expect you to clean up all cases, just Libtasn1, since that's what > this patch is about. I would revert this patch and change the unconditional > check for Libtasn1 in OptionsWPE.cmake to look more like the check in > OptionsGTK.cmake instead. Okay, I'll do that. 1383291 10 timothy 2017-12-19 14:00:56 -0800 (In reply to Michael Catanzaro from comment #8) > (In reply to Timothy Hatcher from comment #7) > > I was hitting a linker or build error (can't recall which, this was a while > > ago) when they were unconditional. I wouldn't have touched this otherwise. > > Maybe just a side effect of our platform at Tesla? > > I don't know. > > I don't expect you to clean up all cases, just Libtasn1, since that's what > this patch is about. I would revert this patch and change the unconditional > check for Libtasn1 in OptionsWPE.cmake to look more like the check in > OptionsGTK.cmake instead. OptionsGTK.cmake and OptionsWPE.cmake are identical in how they require the Libtasn1 package. I don't know what you are wanting to do. 1383343 11 mcatanzaro 2017-12-19 15:00:12 -0800 Oops, I was looking at my stale local checkout! Indeed, no changes are required there. In that case, I'm tempted to say that all we need to do is revert this patch. I don't think reverting it should cause any link errors, as explained above. If it does, I'd like to see the error. 329682 2017-12-18 14:13:53 -0800 2017-12-18 20:09:14 -0800 Patch bug-180949-20171218141353.patch text/plain 2851 timothy U3VidmVyc2lvbiBSZXZpc2lvbjogMjI2MDc3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMWY4MzQxMTE1NjVhNmQy ODc5ZjA4YjEwNTJiNjY0ZjRjMDcxMGU5YS4uMWYwNmY1ZmEzMWM5ZTlhNmU0OTRiNzE5ZGZiZTcy MjJlMmZlNDk0OCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE1IEBACiAyMDE3LTEyLTE4ICBUaW1v dGh5IEhhdGNoZXIgIDx0aW1vdGh5QGhhdGNoZXIubmFtZT4KIAorICAgICAgICBbR1RLXVtXUEVd IENvbmRpdGlvbmFsaXplIGxpYlRBU04xIHVzZSBiZWhpbmQgRU5BQkxFX1NVQlRMRV9DUllQVE8g aW4gdGhlIENNYWtlIGZpbGVzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xODA5NDkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICAqIFBsYXRmb3JtR1RLLmNtYWtlOiBNb3ZlIHRoZSBpbmNsdWRlIHBhdGggYW5k IGxpYnJhcnkgYWRkaXRpb25zIHRvIGNvbmRpdGlvbmFsIEVOQUJMRV9TVUJUTEVfQ1JZUFRPIHNl Y3Rpb24uCisgICAgICAgICogUGxhdGZvcm1XUEUuY21ha2U6IERpdHRvLgorCisyMDE3LTEyLTE4 ICBUaW1vdGh5IEhhdGNoZXIgIDx0aW1vdGh5QGhhdGNoZXIubmFtZT4KKwogICAgICAgICBCdWls ZCBmYWlsdXJlIHdoZW4gRU5BQkxFX1ZJREVPX1RSQUNLIGlzIGRpc2FibGVkCiAgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODA5NDgKIApkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvUGxhdGZvcm1HVEsuY21ha2UgYi9Tb3VyY2UvV2ViQ29yZS9QbGF0 Zm9ybUdUSy5jbWFrZQppbmRleCAxNTMyYmY3MTYzZmZhNjc3MTZlZjMwYjk3YTQ4ZDdhYjQ1ZDU2 OGQwLi41MzRjY2U4ZGUyZDVmNTU0NjMyNDU3YTkwN2FhZGUyOTI1NThlNWQzIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9QbGF0Zm9ybUdUSy5jbWFrZQorKysgYi9Tb3VyY2UvV2ViQ29yZS9Q bGF0Zm9ybUdUSy5jbWFrZQpAQCAtMTA0LDcgKzEwNCw2IEBAIGxpc3QoQVBQRU5EIFdlYkNvcmVf TElCUkFSSUVTCiAgICAgJHtHTElCX0xJQlJBUklFU30KICAgICAke0xJQlNFQ1JFVF9MSUJSQVJJ RVN9CiAgICAgJHtMSUJTT1VQX0xJQlJBUklFU30KLSAgICAke0xJQlRBU04xX0xJQlJBUklFU30K ICAgICAke0hZUEhFTl9MSUJSQVJJRVN9CiAgICAgJHtVUE9XRVJHTElCX0xJQlJBUklFU30KICAg ICAke1gxMV9YMTFfTElCfQpAQCAtMTI2LDcgKzEyNSw2IEBAIGxpc3QoQVBQRU5EIFdlYkNvcmVf U1lTVEVNX0lOQ0xVREVfRElSRUNUT1JJRVMKICAgICAke0dMSUJfSU5DTFVERV9ESVJTfQogICAg ICR7TElCU0VDUkVUX0lOQ0xVREVfRElSU30KICAgICAke0xJQlNPVVBfSU5DTFVERV9ESVJTfQot ICAgICR7TElCVEFTTjFfSU5DTFVERV9ESVJTfQogICAgICR7VVBPV0VSR0xJQl9JTkNMVURFX0RJ UlN9CiAgICAgJHtaTElCX0lOQ0xVREVfRElSU30KICkKQEAgLTE3Miw2ICsxNzAsMTUgQEAgaWYg KEVOQUJMRV9QTFVHSU5fUFJPQ0VTU19HVEsyKQogICAgICkKIGVuZGlmICgpCiAKK2lmIChFTkFC TEVfU1VCVExFX0NSWVBUTykKKyAgICBsaXN0KEFQUEVORCBXZWJDb3JlX1NZU1RFTV9JTkNMVURF X0RJUkVDVE9SSUVTCisgICAgICAgICR7TElCVEFTTjFfSU5DTFVERV9ESVJTfQorICAgICkKKyAg ICBsaXN0KEFQUEVORCBXZWJDb3JlX0xJQlJBUklFUworICAgICAgICAke0xJQlRBU04xX0xJQlJB UklFU30KKyAgICApCitlbmRpZiAoKQorCiBpZiAoRU5BQkxFX1dBWUxBTkRfVEFSR0VUKQogICAg IGxpc3QoQVBQRU5EIFdlYkNvcmVfU1lTVEVNX0lOQ0xVREVfRElSRUNUT1JJRVMKICAgICAgICAg JHtXQVlMQU5EX0lOQ0xVREVfRElSU30KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL1BsYXRm b3JtV1BFLmNtYWtlIGIvU291cmNlL1dlYkNvcmUvUGxhdGZvcm1XUEUuY21ha2UKaW5kZXggZTgx NDVmYTM0MWM4NjU5ZDc4ODhlZDUyMjM0YTU2ZTAxNTJjZWY5My4uNWJiMWRjNmVjZjVhZTdjZWJj NmY1NDI1ZTdlNmQzYWRmZDIzYjI0YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvUGxhdGZv cm1XUEUuY21ha2UKKysrIGIvU291cmNlL1dlYkNvcmUvUGxhdGZvcm1XUEUuY21ha2UKQEAgLTcx LDcgKzcxLDYgQEAgbGlzdChBUFBFTkQgV2ViQ29yZV9MSUJSQVJJRVMKICAgICAke0dMSUJfTElC UkFSSUVTfQogICAgICR7SUNVX0xJQlJBUklFU30KICAgICAke0xJQlNPVVBfTElCUkFSSUVTfQot ICAgICR7TElCVEFTTjFfTElCUkFSSUVTfQogICAgICR7VVBPV0VSR0xJQl9MSUJSQVJJRVN9CiAg ICAgJHtXUEVfTElCUkFSSUVTfQogKQpAQCAtODIsNyArODEsMTUgQEAgbGlzdChBUFBFTkQgV2Vi Q29yZV9JTkNMVURFX0RJUkVDVE9SSUVTCiAgICAgJHtHTElCX0lOQ0xVREVfRElSU30KICAgICAk e0lDVV9JTkNMVURFX0RJUlN9CiAgICAgJHtMSUJTT1VQX0lOQ0xVREVfRElSU30KLSAgICAke0xJ QlRBU04xX0lOQ0xVREVfRElSU30KICAgICAke1VQT1dFUkdMSUJfSU5DTFVERV9ESVJTfQogICAg ICR7V1BFX0lOQ0xVREVfRElSU30KICkKKworaWYgKEVOQUJMRV9TVUJUTEVfQ1JZUFRPKQorICAg IGxpc3QoQVBQRU5EIFdlYkNvcmVfU1lTVEVNX0lOQ0xVREVfRElSRUNUT1JJRVMKKyAgICAgICAg JHtMSUJUQVNOMV9JTkNMVURFX0RJUlN9CisgICAgKQorICAgIGxpc3QoQVBQRU5EIFdlYkNvcmVf TElCUkFSSUVTCisgICAgICAgICR7TElCVEFTTjFfTElCUkFSSUVTfQorICAgICkKK2VuZGlmICgp Cg==