search

sy6sy2 / xbmc

Kodi is an award-winning free and open source home theater/media center software and entertainment hub for digital media. With its beautiful interface and powerful skinning engine, it's available for Android, BSD, Linux, macOS, iOS and Windows.
https://kodi.tv/
Other
6 stars 1 forks source link

Multi threaded depends build still fails sometimes #23

Closed phunkyfish closed 5 years ago

phunkyfish commented 5 years ago

Not as often as before. Must still be a dependency missing somewhere.

fuzzard commented 5 years ago

Can you get a log of a failure. I havent had a failure since the openssl fix was proposed/implemented.

phunkyfish commented 5 years ago

Is there a log or do I need to capture the output?

fuzzard commented 5 years ago

Capture the output. Need to find what module it fails on to work out the fix

sy6sy2 commented 5 years ago

Just build dependencies from scratch with make -j$(getconf _NPROCESSORS_ONLN) (-j4 in my case) and no problem for me. 50 minutes for me

phunkyfish commented 5 years ago

Let me run a few builds over the next few days and I’ll capture a failure.

fuzzard commented 5 years ago

Cheers, no rush. As i said, i havent had a failure since that fix went in, but similar to Sylvain, ive been using -j4, so maybe on a higher threadcount?

phunkyfish commented 5 years ago

It happened on my 2 core MacBook. Might take me a while to reproduce 😉

sy6sy2 commented 5 years ago

Yeah! I managed to have an error xD This is my stdout/stderr log file: make_stdout.txt

Edit:

make[3]: *** [configdata.pm] Error 1
make[2]: *** [appletvos12.2_arm64-target-debug/libssl.a] Error 2
make[1]: *** [openssl] Error 2
make[1]: *** Waiting for unfinished jobs....

On line 35923

sy6sy2 commented 5 years ago

I was able to reproduce the error a second time. Seems to be the same error: make_stdout.txt

fuzzard commented 5 years ago

Cheers, I've been able to get a log myself. I'll work on it.

phunkyfish commented 5 years ago

I havn't managed to repro yet but looks to be the same error from memory.

fuzzard commented 5 years ago

It's definitely the same root issue, just need to work out why. A log I captured had 3 configures done for openssl, and after the first, it just wipes the makefile back to the base one with none of the cflags required. Only just parsed the log before I went out for the night, so I'll look into it more over the next day or so

phunkyfish commented 5 years ago

Fails also happen on single threaded builds: https://github.com/SylvainCecchetto/xbmc/issues/19#issuecomment-484199455

So I kicked off 10 single threaded builds and 10 multi threaded builds. One failed on each and they appear to be the same error each time. I unfortunately forgot to capture the output but is similar to the example errors already on the issue.

sy6sy2 commented 5 years ago

Are sure that is the same error? I can not find fatal error: 'assert.h' file not found #include <assert.h> text in the two files make_stdout that I uploaded above.

phunkyfish commented 5 years ago

You need to rerun make for that error to appear. It won’t appear on the first run.

sy6sy2 commented 5 years ago

Hum ok. Is it an expected behavior to not print this message on the first run?

phunkyfish commented 5 years ago

Will confirm next time it occurs on a multithreaded build.

Afraid I don’t know about the expected behaviour.

fuzzard commented 5 years ago

the following from the ios.md build instructions makes me wonder if this isnt an existing issue

WARNING: Look for the Dependencies built successfully. success message. If in doubt run a single threaded make command until the message appears. If the single make fails, clean the specific library by issuing make -C target/ distclean and run makeagain.

I'm going to just run some repeated tests against master to see if i can get it to fail on an ios build. May just be a kodi gremlin that we are seeing often due to the state of the work. (ie rebuilding depends constantly)

fuzzard commented 5 years ago

and easiest way to confirm its the same issue, go into tools/depends/target/openssl/appletvos12.2_arm64-target-debug and look at the Makefile. Specifically look at the CFLAGS, and then compare to Makefilee (backup created by sed)

note: directory may be different due to sdk version, so just go into openssl and then the directory inside that

phunkyfish commented 5 years ago

So is the issue that the backup and the Makefile are the same, I.e. the sed command is failing to make any change?

Or it’s being overwritten after sed is run?

fuzzard commented 5 years ago

No, so the backupfile is created by the sed -E .... command

sed -E -ie "s|^CFLAGS=-|CFLAGS=$(CFLAGS) -|" "$(PLATFORM)/Makefile"; \

On the parse of the Makefile, the backup is created by the above sed command. Each time the configure is modified (eg Line 56), autoconf reruns which recreates Configure. What the issue we are facing is that the openssl Configure is being modifed (3 times in the failures i have), which recreates the Configure script with the default CFLAGS, and the makefile isnt run multiple times to modify the CFLAGS at L57 https://github.com/SylvainCecchetto/xbmc/blob/a305a03c27a8319dbe6bd3158c0f2b6dc1f4e98a/tools/depends/target/openssl/Makefile#L57

What needs to be worked out is how/why the Configure script is being modified after the first time to cause the multiple autoreconf's. All configure changes need to be made/completed before we get to the sed block that modifies the CFLAGS so no more autoreconf runs are done to blow away the CFLAGS

fuzzard commented 5 years ago

Ok, i have a proposed change, but i need confirmation from someone who can do a full build and deploy to an ATV. If someone can apply the following diff and do a full build/deploy to make sure everything still works as i expect. I dont have an ATV4 at all otherwise i would have tested myself. WIth the change i cannot trigger the reconfigure issue, whereas without i can trigger quite reliably every 3-4 builds of openssl.

diff --git a/tools/depends/target/openssl/Makefile b/tools/depends/target/openssl/Makefile
index feb22e8..d0cb56b 100644
--- a/tools/depends/target/openssl/Makefile
+++ b/tools/depends/target/openssl/Makefile
@@ -53,7 +53,6 @@ $(PLATFORM): $(TARBALLS_LOCATION)/$(ARCHIVE) $(DEPS)

        # Stolen pathes here: https://gist.github.com/felix-schwarz/c61c0f7d9ab60f53ebb0 
        if test "$(OS)" = "tvos"; then \
-           sed -i -- 's/D\_REENTRANT\:iOS/D\_REENTRANT\:tvOS/' "$(PLATFORM)/Configure"; \
                sed -E -ie "s|^CFLAGS=-|CFLAGS=$(CFLAGS) -|" "$(PLATFORM)/Makefile"; \
                sed -ie "s|-isysroot \$$(CROSS_TOP)/SDKs/\$$(CROSS_SDK) ||" "$(PLATFORM)/Makefile"; \
                sed -ie "s|static volatile sig_atomic_t intr_signal;|static volatile intr_signal;|" "$(PLATFORM)/crypto/ui/ui_openssl.c"; \

The reasoning behind this. I believe the reentrant requirement was from openssl <= 1.0.2. OpenSSL 1.1.0h does not differentiate with the reentrant flag at all (no iOS or tvOS, just D_REENTRANT), so that SED command is not actually changing anything anyway. I believe removing it will have no affect on a full build.

Digging in as far as i can right now, the D_REENTRANT flag isnt being used for any iphoneos-cross (or the apparently preferred ios64-cross), and in Darwin, its only used for MacOS darwin-common. ie not relevant to us at all.

phunkyfish commented 5 years ago

In case this was a kodi gremlin I started as many MacOSX builds as I could last night. However all builds succeeded.

This supports your REENTERANT thinking. Running a build now. If it finishes before I go to work will test also.

sy6sy2 commented 5 years ago

Hi, I will try your diff too.

Before sleeping last night I started a single treated make on the ATV-PR branch. When I woke up I had:

[...]
THIRTY_TWO_BIT mode
BN_LLONG mode
RC4 uses unsigned char

Configured for iphoneos-cross.
**************************************************
***                                            ***
***   Please run the same make command again   ***
***                                            ***
**************************************************
make[3]: *** [configdata.pm] Error 1
make[2]: *** [appletvos12.2_arm64-target-debug/libssl.a] Error 2
make[1]: *** [openssl] Error 2
make: *** [target/.installed-appletvos12.2_arm64-target-debug] Error 2

Then I did make -C target/openssl and I get:

[...]
crypto/aes/aes_core.c:39:10: fatal error: 'assert.h' file not found
#include <assert.h>
         ^~~~~~~~~~
1 error generated.
make[2]: *** [crypto/aes/aes_core.o] Error 1
make[1]: *** [all] Error 2
make: *** [appletvos12.2_arm64-target-debug/libssl.a] Error 2
phunkyfish commented 5 years ago

Yes, this is the same as mine above, a bad sysroot definition.

My build is nearly ready to test.

phunkyfish commented 5 years ago

Everything appears to work just fine 😉

fuzzard commented 5 years ago

Sweet. I'll send a PR to the ATV branch in a couple hours. Will need to update the depends branch to remove it as well, let me know if you need me to do anything there Sylvain

sy6sy2 commented 5 years ago

On my side I test on the ATV-PR branch with OS=ios and TARGET_PLATFORM=appletvos. So my openssl Makefile looks like that with your fix:

[...]

ifeq ($(OS), ios)
  ifeq ($(TARGET_PLATFORM),appletvos)
    # Need to add "no-async" to avoid "'setcontext' is unavailable: not available on tvOS" error
    CONFIGURE=./Configure iphoneos-cross no-shared zlib no-asm no-async --prefix=$(PREFIX)
  else
    CONFIGURE=./Configure iphoneos-cross no-shared zlib no-asm --prefix=$(PREFIX)
  endif
endif

[...]

$(PLATFORM): $(TARBALLS_LOCATION)/$(ARCHIVE) $(DEPS)
    rm -rf $(PLATFORM); mkdir -p $(PLATFORM)
    cd $(PLATFORM); $(ARCHIVE_TOOL) $(ARCHIVE_TOOL_FLAGS) $(TARBALLS_LOCATION)/$(ARCHIVE)
    cd $(PLATFORM); AR="$(AR)" CFLAGS="$(CFLAGS)" CC="$(CC)" RANLIB=$(RANLIB) $(CONFIGURE)
    if test "$(OS)" = "osx"; then \
        sed -ie "s|CC= /usr/bin/gcc-4.2|CC= $(CC)|" "$(PLATFORM)/Makefile"; \
        sed -E -ie "s|^CFLAGS=-|CFLAGS=$(CFLAGS) -|" "$(PLATFORM)/Makefile"; \
    fi
    # for iphoneos-cross config a sysroot argument is added
    # however sysroot already set in Makefile.include, so remove this
    if test "$(OS)" = "ios"; then \
        sed -E -ie "s|^CFLAGS=-|CFLAGS=$(CFLAGS) -|" "$(PLATFORM)/Makefile"; \
        sed -ie "s|-isysroot \$$(CROSS_TOP)/SDKs/\$$(CROSS_SDK) ||" "$(PLATFORM)/Makefile"; \
        sed -ie "s|static volatile sig_atomic_t intr_signal;|static volatile intr_signal;|" "$(PLATFORM)/crypto/ui/ui_openssl.c"; \
    fi

    # Stolen pathes here: https://gist.github.com/felix-schwarz/c61c0f7d9ab60f53ebb0
    if test "$(TARGET_PLATFORM)" = "appletvos"; then \
        sed -i -- 's/define HAVE_FORK 1/define HAVE_FORK 0/' "$(PLATFORM)/apps/speed.c"; \
    fi

    sed -ie "s|apps test||" "$(PLATFORM)/Makefile"; \

I will try to build multiple times dependencies with this fix and with -j4

sy6sy2 commented 5 years ago

(I just see your message fuzzard, is your fix looks good here?) If yes and if my tests work, I can modify this file on the current Pull request.

fuzzard commented 5 years ago

Yeah, I'm confident it will fix the issue with no consequences.

sy6sy2 commented 5 years ago

Ok!

Not related, but I think that at some time we will need to apply changes from ATV-PR branch to ATV branch no? (I mean in the ATV branch we use OS=tvOS whereas we use OS=ios and TARGET_PLATFORM=appletvos on the ATV-PR branch).

fuzzard commented 5 years ago 
    # Stolen pathes here: https://gist.github.com/felix-schwarz/c61c0f7d9ab60f53ebb0
    if test "$(TARGET_PLATFORM)" = "appletvos"; then \
        sed -E -ie "s|^CFLAGS=-|CFLAGS=$(CFLAGS) -|" "$(PLATFORM)/Makefile"; \
        sed -ie "s|-isysroot \$$(CROSS_TOP)/SDKs/\$$(CROSS_SDK) ||" "$(PLATFORM)/Makefile"; \
        sed -ie "s|static volatile sig_atomic_t intr_signal;|static volatile intr_signal;|" "$(PLATFORM)/crypto/ui/ui_openssl.c"; \
        sed -i -- 's/define HAVE_FORK 1/define HAVE_FORK 0/' "$(PLATFORM)/apps/speed.c"; \
    fi

Should be what you want in the openssl makefile on the depends branch Sylvain

fuzzard commented 5 years ago

Yeah, assuming we can get the depends branch merged, there is going to be a little bit of work rebasing, I haven't even looked down that path yet

sy6sy2 commented 5 years ago

Is the if test "$(OS)" = "ios"; above this block does not already apply sed on CLFAGS, isysroot and static volatile?

If I copy paste your block some same sed command are applied two times isn't it?

fuzzard commented 5 years ago

Could be, sorry on mobile at the moment.

phunkyfish commented 5 years ago

@SylvainCecchetto yes, you’re correct. We only want it applied once.

sy6sy2 commented 5 years ago

No problem fuzzard.

So, from scratch, first run on ATV-PR with your fix and with make -j4 --> Dependencies built successfully.

Let's go with a make distclean in order to perform a second test!

phunkyfish commented 5 years ago

Did we include the make distclean fix in the PR. Or maybe I’m getting confused with the fix for binary addons.

sy6sy2 commented 5 years ago

Oh no, sorry, I did not express myself well, I just wanted to say that I started a second dependencies build test with a clean base.

So, once again I got a Dependencies built successfully. with a make -j4 in tools/depends (40 minutes).

I will try again just to be sure and if it's still OK I will modify the file openssl/Makefile on the ATV-PR branch BUT I will not touch the file in ATV branch.

Edit: This is the modified version with buzzard's fix that I use on ATV-PR-depends: Makefile_openssl_APT-PR-depends.txt

(Just to be clear, I only tested to build dependencies and not Kodi core with Xcode)

fuzzard commented 5 years ago

Have sent a PR to the ATV-PR-Depends branch @SylvainCecchetto

Feel free to pick/close.

sy6sy2 commented 5 years ago

Thank you, just applied and force push changes of openssl/Makefile file on ATV-PR-depends branch --> https://github.com/xbmc/xbmc/pull/15919/files#diff-e77f268c9e6f35c2cd7a9fa863a270e8

phunkyfish commented 5 years ago

Looks good

sy6sy2 commented 5 years ago

Cool!

So, it's now the third time I've been able to build dependencies with make -j4 without error (on ATV-PR-depends). it seems to be good! Thank you for the fix.

phunkyfish commented 5 years ago

Nice!

sy6sy2 commented 5 years ago

Can we close this issue (and cross fingers to not reopen it)?

phunkyfish commented 5 years ago

Yes, let’s close.