google / cdep

CDep is a decentralized native package dependency manager with a focus on Android. Runs on Windows, Linux, and MacOS.
Apache License 2.0
98 stars 22 forks source link

Update to add v8. #50

Closed gpx1000 closed 7 years ago

gpx1000 commented 7 years ago

Add to the smoke test list.

jomof commented 7 years ago

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

jomof commented 7 years ago

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 commented 7 years ago

Hmm... I know what happened. I did more than one publish in the forked v8 repo. It looks like after I renamed it to v8-cdep when I went back and renamed it to v8 it's stepping on the old publish. So I need to find that original publish and nuke it. Everything else looks right.

On Mon, Sep 25, 2017 at 10:25 AM, Jomo Fisher notifications@github.com wrote:

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

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/google/cdep/pull/50#issuecomment-331952647, or mute the thread https://github.com/notifications/unsubscribe-auth/ADqAYySBpVyJAbfElIWgWLEKVZJX-QPhks5sl-IbgaJpZM4PjDoS .

gpx1000 commented 7 years ago

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 commented 7 years ago

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

gpx1000 commented 7 years ago

Sounds good, I'll try to do both at some point in the future.

On Mon, Sep 25, 2017 at 11:11 AM, Jomo Fisher notifications@github.com wrote:

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

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/google/cdep/pull/50#issuecomment-331966148, or mute the thread https://github.com/notifications/unsubscribe-auth/ADqAY8ESS1Y8r7oYzkJCaiufdepp0igiks5sl-y9gaJpZM4PjDoS .

gpx1000 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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!

gpx1000 commented 7 years ago

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

gpx1000 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

gpx1000 commented 7 years ago

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.

jomof commented 7 years ago

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 commented 7 years ago

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.

jomof commented 7 years ago

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 commented 7 years ago

[ 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 commented 7 years ago

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

gpx1000 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 ;-).

gpx1000 commented 7 years ago

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 commented 7 years ago

Thanks!