Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to add v8. #50

Merged
merged 14 commits into from
Oct 10, 2017
Merged

Update to add v8. #50

merged 14 commits into from
Oct 10, 2017

Conversation

gpx1000
Copy link
Collaborator

@gpx1000 gpx1000 commented Sep 25, 2017

Add to the smoke test list.

@jomof
Copy link
Collaborator

jomof commented Sep 25, 2017

Looks like Travis failed. I'll take a peek and see if I can tell what happened

@jomof
Copy link
Collaborator

jomof commented Sep 25, 2017

Error from Travis:
/home/travis/build/google/cdep/smoke-test/././downloaded-packages/downloads/com.github.gpx1000/v8/5.5.372/cdep-manifest.yml:3: error: artifactId 'v8' from manifest did not agree with 'v8-armeabi' from github url https://github.com/gpx1000/v8/releases/download/5.5.372/cdep-manifest.yml [CDEP51077cd]

You'll have to update the manifest to match the name of the github repo

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 25, 2017 via email

This should force the cache to forget about the now deleted release;  updating the tag that v8 uses.
@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 25, 2017

CMake Error at .cdep/modules/cdep-dependencies-config.cmake:1164 (message):
Android ABI armeabi is not supported by com.github.gpx1000:v8:5.5.372-cdep
for platform 21. Supported: armeabi-v7a arm64-v8a x86

Looks like this will have to wait until I get a chance to add a few more ABIs.

@jomof
Copy link
Collaborator

jomof commented Sep 25, 2017

I'm okay with cutting armeabi from this test, but I think we'd need x86_64.

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 25, 2017 via email

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 27, 2017

Supported: x86_64 armeabi-v7a arm64-v8a x86

Excluding mips, those are all the ones that v8 has a build for that are android. (At least I believe that arm.release formula for gn is creating an armeabi-v7a and not a armeabi). ;-)

@jomof
Copy link
Collaborator

jomof commented Sep 27, 2017

Can you support armeabi? If not, I'm open to drop it from the test.

Any chance could get rid of the final "cdep" in the version?

com.github.gpx1000:v8:6.3.216-cdep -> com.github.gpx1000:v8:6.3.216

The trailing dash-fragment is supposed to be for a version for your local build (where the v8 build number stays constant).

Sorry I've been slow to respond, I'm fighting a cold

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 27, 2017

it looks like I messed up, armeabi is what arm.release outputs. So the correct label would be armeabi and no armeabi-v7a. I'll create a new release and try to get it right this time.

Hope you feel better!

2.) correct the release label to drop -cdep specifier.
@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 28, 2017

hit commit too soon, now those files are up. committing again.

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 28, 2017

CMake Error at .cdep/modules/cdep-dependencies-config.cmake:3005 (message):
Android ABI armeabi-v7a is not supported by com.github.gpx1000:v8:6.3.2160
for platform 21. Supported: x86_64 armeabi arm64-v8a x86

Also looks like I need to add in missing cflags; smoke test is a good test; need to fix the example for this new version.

@jomof
Copy link
Collaborator

jomof commented Sep 28, 2017

You'd need to declare which compiler features your package needs. For example,

interfaces:
headers:
....
requires: [cxx_nullptr]

Full list of compiler features is here. https://cmake.org/cmake/help/v3.9/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#prop_gbl:CMAKE_CXX_KNOWN_FEATURES

Note that you can jump straight to c++11 the same way with cxx_std_11. It may cut off some scenarios (maybe)

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 28, 2017

Ah, that makes sense, thanks! I wonder if there's a possible way to make an auto generator as not all libraries use CMake; and then not all of those are going to define features.

@jomof
Copy link
Collaborator

jomof commented Sep 28, 2017

It also works for ndk-build but most features promote straight to the minimum language required (equivalent of cxx_std_11)

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 28, 2017

It makes sense on the consumer side. However, when authoring packages; I can see how that could be a slight problem on the generation of the authoring of cdep-manifest.yml. Ideally, there'd be a switch to tell cmake/ndk-build to output what was required when the library is built.
The headache I'm imagining is in maintenance, when it's time to create a new package, the process would include discovering any new changes to the features. I'm not entirely sure there's an easy answer to this.

Add in link to c++ features.
@jomof
Copy link
Collaborator

jomof commented Sep 28, 2017

I see what you mean, it isn't easy to figure out the exact set of require flags. I expect many package authors to promote cxx_std_11 (or higher). This can generally be known by looking at compiler flags (like std=c++11). Going by individual flags only makes sense if there is a need to be truly granular and there really isn't on Android platform.

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 28, 2017

There is one further use case. The current SDK ships with two cmake binaries; cxx_std_11 was added to the list in version 3.7 of CMake. I added docs suggesting using the SDK's cmake; however, obviously can't promise that everyone isn't going to choose using the 3.6 cmake when creating a lib in a standalone project we'd want to include.
I'm imagining this is something that we're going to have to figure out logic for in a CPack or something.

Fix the example so it can build.
@jomof
Copy link
Collaborator

jomof commented Sep 28, 2017

Oh interesting point about our version of CMake not supporting cxx_std_11. We are working on removing the requirement to use our fork of CMake.

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 29, 2017

[ 94%] Linking CXX shared library ../../../../../staging/lib/armeabi/libv8_target.so
/home/travis/build/google/cdep/smoke-test/.cmakeify/tools/android-ndk-r13b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/lib/gcc/arm-linux-androideabi/4.9.x/../../../../arm-linux-androideabi/bin/ld: error: [ 94%] Built target stb_rect_pack_target
/home/travis/build/google/cdep/smoke-test/././downloaded-packages/exploded/com.github.gpx1000/v8/6.3.2162/v8-armeabi.zip/lib/armeabi/libv8_libplatform.a(default-platform.o): incompatible target
/home/travis/build/google/cdep/smoke-test/.cmakeify/tools/android-ndk-r13b/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/lib/gcc/arm-linux-androideabi/4.9.x/../../../../arm-linux-androideabi/bin/ld: error: /home/travis/build/google/cdep/smoke-test/././downloaded-packages/exploded/com.github.gpx1000/v8/6.3.2162/v8-armeabi.zip/lib/armeabi/libv8_base.a(api.o): incompatible target

That's the only one that I didn't test locally. The rest should pass the build. I'll submit again when I get back next week.

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Sep 29, 2017

I've noticed that; which should solve the problem.

Add release for armeabi to hopefully link correctly.
@gpx1000
Copy link
Collaborator Author

gpx1000 commented Oct 2, 2017

Looks like it all passed other than missing armeabi-v7a build. It does have armeabi, and I'm not certain there's a way to target armeabi-v7a in v8 directly.

@jomof
Copy link
Collaborator

jomof commented Oct 9, 2017

Hi Steven,
I'm back from vacation and trying to catch up. Hmmm, armeabi-v7a is prettty mainstream. How do people make useful apps built on v8 without this? In any case, should we pull it from this CL? It would still be available just not smoke-tested.

@gpx1000
Copy link
Collaborator Author

gpx1000 commented Oct 10, 2017

Hi Jomo,
Welcome back!
armv7a is what I expect as default for any "android" build; I haven't seen many libs distributed that only build armeabi or arm64. Then again, I've seen plenty of libraries release armeabi with the expectation that it will work on armv7.
The issue is the way that you build it is with the following gn command:
gn args android-arm.release. If I were to guess it would require setting the cpu in gn args to armv7a or something like it. I admittedly didn't spend a lot of time trying to decipher the build so it likely means something like that is missing.
I'm more than happy to pull it from this CL. Entire exercise was me getting familiar with CDep ;-).

Remove V8 from smoke test
@gpx1000
Copy link
Collaborator Author

gpx1000 commented Oct 10, 2017

I'm going to investigate how to get an armeabi-v7a build done and will then add back to the smoke test before the next version of V8 is released. For now as mentioned pulled it from the smoke test (left zlib in).

@jomof jomof merged commit 4bb3f21 into google:master Oct 10, 2017
@jomof
Copy link
Collaborator

jomof commented Oct 10, 2017

Thanks!

@gpx1000 gpx1000 mentioned this pull request Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants