Skip to content

Commit

Permalink
Fix configure glog script when building from xcodebuild
Browse files Browse the repository at this point in the history 
Summary:
I encountered an issue when building with fastlane gym / xcodebuild where glog would not build because of missing config.h header file. I tracked it down to the ios-configure-glog.sh script that ended up error-ing because of missing valid c compiler. I guess it didn't enter the if to set c compiler env in xcodebuild and that env doesn't have proper values set like it does in xcode so just removing this check fixed it. Also tested that it still works properly in xcode.
Closes #14267

Differential Revision: D5285691

Pulled By: javache

fbshipit-source-id: df5315926c2d2d78806618df3d9c9bbbb974d1ea
  • Loading branch information
@janicduplessis @facebook-github-bot
janicduplessis authored and facebook-github-bot committed Jun 21, 2017
1 parent 1230549 commit 5c53f89
Showing 1 changed file with 5 additions and 5 deletions.
10 changes: 5 additions & 5 deletions scripts/ios-configure-glog.sh
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
#!/bin/bash
set -e

# Only set when not running in an Xcode context
if [ -z "$ACTION" ] || [ -z "$BUILD_DIR" ]; then
export CC="$(xcrun -find -sdk iphoneos cc) -arch armv7 -isysroot $(xcrun -sdk iphoneos --show-sdk-path)"
export CXX="$CC"
fi
PLATFORM_NAME="${PLATFORM_NAME:-iphoneos}"
CURRENT_ARCH="${CURRENT_ARCH:-armv7}"

export CC="$(xcrun -find -sdk $PLATFORM_NAME cc) -arch $CURRENT_ARCH -isysroot $(xcrun -sdk $PLATFORM_NAME --show-sdk-path)"
export CXX="$CC"

./configure --host arm-apple-darwin

Expand Down

19 comments on commit 5c53f89

@SudoPlz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That commit just fixed my CI failures, and spared me hours of headaches.

Thanks!

@sjmueller
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix!

@SudoPlz
Copy link
Contributor

@SudoPlz SudoPlz commented on 5c53f89 Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only problem is, that commit doesn't seem to be found in 0.46.0 or 0.46.1 so we still have to wait for 0.47.* to come out.

@ujwal-setlur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second that this needs to come out ASAP and not wait for 0.47. 0.46 is not usable until this is fixed.

@SudoPlz
Copy link
Contributor

@SudoPlz SudoPlz commented on 5c53f89 Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running the following script on npm post install seems to have fixed my problem:

cd node_modules/react-native/scripts/
curl https://raw.githubusercontent.com/facebook/react-native/5c53f89dd86160301feee024bce4ce0c89e8c187/scripts/ios-configure-glog.sh >ios-configure-glog.sh
chmod +x ios-configure-glog.sh

@ujwal-setlur
Copy link

@ujwal-setlur ujwal-setlur commented on 5c53f89 Jul 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the heck is this fix coming out??? Even 0.46.2 does not have this change. I apologize if I am sounding impatient, but this is completely blocking an upgrade to rn-0.46. Why is this not getting rolled out? Hopefully, someone is aware that 0.45 and 0.46 are completely broken on iOS???

@ujwal-setlur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, I know manually running the script or making this code change will resolve it, but still, I would have hoped that such a critical bug fix would have been rolled out soon.

@romanenko
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this could have been merged into some 0.46.* patch version. Without it our CI builds are broken too.

@hramos
Copy link
Contributor

@hramos hramos commented on 5c53f89 Jul 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can nominate commits to be cherry picked by searching for the release number in the issues. The task tracking commits that should be cherry picked into a patch release for 0.46 is at #14713

Please be patient and use these tracking issues to let the maintainers know of anything that needs to be patched. We don't get notified of comments left on commits.

@ujwal-setlur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hramos

Thanks a lot for this tip. I was unaware of this task tracking commits.

Apologies for my impatience and frustration!

@ujwal-setlur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.46.3 is out with this fixed. Thanks @hramos!

@ujwal-setlur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janicduplessis, @hramos, @javache

I believe this file ios-configure-glog.sh needs another line:

rm -f test-driver

or

unlink test-driver

This is because the the tar.gz file, when expanded has this link that messes up the android build referenced by #14464, #14417, #14548

These issues occur if an android build is attempted after an iOS build. This link is not required for the iOS build and can be safely removed.

@ujwal-setlur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this is already fixed in master. Wish this could have been cherry-picked as well :)

@hramos
Copy link
Contributor

@hramos hramos commented on 5c53f89 Jul 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's too late for 0.46.3 now. Hopefully we can keep better track of these issues in the 0.47 RC, so we can cherry-pick commits prior to cutting a stable release.

@ujwal-setlur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know :(. I ran into this after I upgraded to 0.46.3. I have created a script to work around this for now. I have updated the other android bugs that this is resolved in master. Hopefully, we don't get breaking changes like this again. I would love to contribute to adding test cases to cover things like this. I really love react-native and definitely want to have the user experience get better with each release :)

@SudoPlz
Copy link
Contributor

@SudoPlz SudoPlz commented on 5c53f89 Jul 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ujwal-setlur feel free to create a pull request bro, that's very generous of you, and would benefit all of us!

@ujwal-setlur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SudoPlz, would be happy to, but not sure if I need to? The issue is fixed in master already via 7d1981e. I have updated #14713 requesting to include that commit in case a 0.46.4 is generated. In the meantime, my workaround is very similar to your #14382 (comment). Here is my version for resolving the android build issue:

	curl https://raw.githubusercontent.com/facebook/react-native/7d1981ef8245cd6267823a970054cbebb2d2fd14/scripts/ios-configure-glog.sh > $PWD/node_modules/react-native/scripts/ios-configure-glog.sh
	chmod +x $PWD/node_modules/react-native/scripts/ios-configure-glog.sh
	printf "\nManually upgraded the ios-configure-glog.sh script.\n"

I saved this as a postinstall script in package.json

@ujwal-setlur
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, I will look at a generating a PR to help cover a bit more testing :)

@mdstroebel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed in 0.46.4 now.

Please sign in to comment.