Many updates - see individual commits #97
Conversation
Due to a clang bug, the 15.x SDK sys/cdefs.h failed to compile with the XCode 7.2 clang 7 on OS 10.10. This adds a workaround for that. This bug is also present in some gcc versions, so the workaround is applied there as well, albeit with an apparently unavoidable warning. TESTED: The checksdkversion and darwin_c_all tests now build and pass with the 15.x SDK and the usual Xcode compiler on all OS versions, as well as all non-problematic MacPorts compilers tested.
Some recent compilers have started treating undefined macros as errors, causing problems with AvailabilityMacros when TARGET_OS_OSX is undefined. We work around that by temporarily defining it if necessary. TESTED: Affected AvailabilityMacros headers now build with all MacPorts compilers tested, as well as with the usual Xcode compilers for all OS versions.
Some recent compilers have started treating undefined macros as errors, causing problems with stdio/stdlib when TARGET_OS_EMBEDDED or TARGET_OS_IPHONE is undefined. We work around that by temporarily defining either if necessary. TESTED: The relevant stdio/stdlib headers now build with all MacPorts compilers tested, as well as with the usual Xcode compilers for all OS versions.
Verifies that math.h can be compiled. Indicates whether _Float16 is defined as a macro (SDK 15 workaround). Optionally reports compiler and architecture info. Since the SDK 15 math.h broke other tests, there's no benefit to placing this commit after the fix in the commit sequence. TESTED: On all platforms, correctly reports all three passing cases, and of course fails to build in problem cases.
Although Apple usually provides workarounds for newer SDKs with older compilers, they neglected to do that for the prototypes of the new half-precision floating-point functions added in macOS 15.0. With older compilers, this results in failed builds, even for code that doesn't use the new functions. Here we provide a workaround for that. Also makes the _Float16 support status available as a macro, regardless of SDK version. TESTED: Xcode compilers for all OS versions, as well as many others, now successfully build math.h from all SDKs.
As of SDK 15, MacTypes.h is relying on __has_include(), but the dummy false definition provided by sys/cdefs.h then causes it to fail to include ConditionalMacros.h, resulting in a build failure. This change provides proper tracking of the __has_include() status, both originally and as potentially modified by sys/cdefs.h. This is then used to temporarily force it true to get the necessary include. This was first observed in an indirect include from IOKit/usb/USB.h. TESTED: All OS Xcode compilers, as well as all others tested, now successfully build IOKit/usb/USB.h from all SDKs.
Unlike the C manual tests, these use the library as usual, but are made manual due to recent stricter compiler/SDK requirements for successful C++ builds. Keeping C++ tests out of the normal automatic test suite allows success on the full range of compilers and SDKs. TESTED: The C++ tests are now run when specified explicitly.
This avoids some C++ problems introduced with the 15.x SDK. The original C++ test is retained as a manual test. A comment claims that this test needed to be in C++, but the alleged explanation is nonexistent. TESTED: Passes on all OS versions with all "non-earlier" SDKs.
The C++ test for dirent.h was intended to test a former problem that no longer exists in the new fdopendir() implementation. Since C++ tests have become somewhat problematic, that test has been moved to the manual tests. Given that it's now rather pointless, it's been replaced with a minimal test of just the include, rather than rewriting the whole test in C. TESTED: Passes on all OS versions with all SDKs.
Referencing "kern.boottime" via sysctlbyname() on 10.4 doesn't work, though it works to use the numeric constants with sysctl(). It was always really dumb to be doing a string lookup on every call, anyway, and this is a speed improvement on all relevant platforms. TESTED: CLOCK_MONOTONIC now works and gives plausible results on all platforms.
This avoids some C++ problems introduced with the 15.x SDK. The original C++ test is retained as a manual test. This version also treats failure returns from the calls being tested as errors. The C++ version simply output the failing return values to the terminal and then "succeeded". The lack of such checks in the old version covered up the fact that CLOCK_MONOTONIC was broken on 10.4. This is now fixed in the previous commit. TESTED: Passes on all OS versions with all "non-earlier" SDKs.
This provides a single make target for all tests that don't require the library. TESTED: Runs all expected tests.
These tests don't need the library. TESTED: Now run as part of xtest target, with no library build.
All automatic C++ tests have now been converted to C, to defend against Apple's increased compiler pickiness in SDK 15. TESTED: The "test" target still runs all automatic tests.
This facilitates building object files and libraries with separate make commands, using different options. This is just for debugging. TESTED: New targets build as expected.
This removes the 'dlib' dependency from the 'syslib' target, since 'syslib' is linked independently from 'dlib'. Also adds the missing '.PHONY' for 'syslib'. TESTED: The 'syslib' target still builds, without unnecessarily building the 'dlib' target.
Some (or even many) object files are logically empty in a given build. These are now excluded from linking the static library, which not only makes the result a bit smaller (while equivalent), but also avoids the "no symbols" warnings when building it. Due to the kludgy way fdopendir.o is constructed, this method doesn't filter it out when it's logically empty (10.10+). We ignore that for now, since it simply means that the old behavior remains for that one object, and we may improve its creation in the future. TESTED: Empty objects are filtered as intended (except as noted above), and all statically-linked tests (added in a subsequent commit) pass.
TESTED: All such tests build as expected and pass (after fixing the symbol-alias test in a subsequent commit).
The old symbol aliases test relied on looking up symbols with dlsym(), which doesn't work with a statically-linked library. This rewrites it to use ordinary external references, which will fail if the referenced symbols are missing. This means that any errors are build-time errors rather than runtime errors. As noted in the comment, there's theoretically a way to avoid that by using weak references, but we don't bother to wrestle with the added complications of that approach. TESTED: This version passes with both static and dynamic libraries.
Creating directories with "install -d" suffers from a race condition, because, although it first-order avoids creating directories that already exist, if multiple instances attempt to create a given directory at the same time, the error due to a collision is not correctly handled. To get around this, we make separate recipes for directory creation, with dependencies on them in the rules that populate them. Unfortunately, this makes the automatic variables useless for some of the populating commands, since they include the directories. In such cases, we need to use explicit variables for the input files. Maintaining the unused ability to put static and dynamic libraries in separate directories (while not actually being separate) would have complicated things, so that capability has been dropped. This special treatment of directories is not applied to the installation of headers, since that's a complicated multi-directory target only used in a single recipe, and hence not vulnerable to parallelism. TESTED: All builds now succeed with parallelism, including those that previously exhibited a failure.
The result of a function being tested is discarded, which produces a "set but not used" warning from some compilers. We add a void cast to avoid this. TESTED: Previously present warnings are now gone.
Six source files contained "macports_legacy_" prefixes in their filenames, which is redundant and overly verbose. This removes those prefixes. No Makefile adjustment is needed, since it relies on wildcards. TESTED: Build succeeds and tests pass.
The syslib targets run versions of the tests linked against the "system" version of the library, substituting for the normal OS system library. This provides a check that the syslib was built correctly. Targets for all syslib tests and all versions of all tests are added. TESTED: Tests build and pass on all platforms. New targets run all expected tests.
|
@mascguy The @MarcusCalhoun-Lopez @macportsraf For both of the latter, see |
|
Agreed. Since the |
|
While I don't have any concerns with the changes, it would probably be good to have at least one other person review them as well: Given how much is included, it's challenging to scrutinize it all. (Definitely not a criticism though, as it's all good stuff.) |
I don't have any problem with more reviewers, as long as it can be done in a timely fashion. And I specifically requested feedback from @MarcusCalhoun-Lopez regarding the inconsistency between the comments and the reality for the Getting too carried away with reviewers can have its problems, though. One time at Google I needed to make a change that both had the potential to screw up thousands of machines and was sufficiently time-critical that it couldn't afford to wait for the normal graduated rollout process. The manager made having several experienced reviewers a condition for short-circuiting the rollout, but then the code review degenerated into arguments among the reviewers themselves over style rather than substance. :-) |
|
Fred, do you need feedback from @MarcusCalhoun-Lopez before we merge? It would be great to get these changes out to a small group via |
It's not essential, but I thought it would be useful to get clarification on the comment that the
If you just mean creating a If you mean issuing a call for more widespread testing via I also belatedly realized that the renaming I did to the overly verbose filenames would require updating the The history of the
Moving the file in the repo instead of deleting it in the Portfile (which just maintains the status quo) means that the Portfile-based build and the bare build in the repo will have the same end result for the first time on over 2.5 years (for the platforms where the wrapper is applicable). |
This has the same effect as what the Portfile has been doing all along, but in a less kludgy manner. TESTED: Has the desired effect on the sources.
The fix for parallel builds made directories separate targets, but directory dependencies need to be order-only, since their mtimes are updated whenever files are added, removed, or renamed. Failure to do this resulted in intermittent unnecessary rebuilds of libraries when building tests. When building directly in the repo, this is a harmless waste of time, but when building in the MacPorts context, such rebuilds fail due to ownership and permission issues. This change makes directories order-only dependencies, thereby ignoring their mtimes. This also removes unnecessary directory dependencies from the tests that don't use the library. TESTED: Ran tests on all platforms. Intermittent test failures are now gone.
|
@mascguy The updated PR is fully tested on all platforms, including with a locally modified Portfile. |
|
@mascguy |
Notably:
Tested on: