intel / external-mesa

16 stars 57 forks source link

Building enhancement #115

Closed renchenglei closed 5 years ago

renchenglei commented 5 years ago

Please don't merge this patch set. I put them here for discussion! On latest Android, google has enhanced its build system. Currently we have many broken rules in mesa project, like implicit rules, using forbidden host tools, etc. See internal Jira Ticket: OAM-80644 OAM-80463 OAM-80307 Qiming help shared some of Google internal fix for those issue: https://android-review.googlesource.com/c/platform/external/mesa3d/+/706190 (Part of merged on upstream) https://android-review.googlesource.com/c/platform/external/mesa3d/+/720960 (Still not included) https://android-review.googlesource.com/c/platform/external/mesa3d/+/761950 (Included as BSP diff) https://android-review.googlesource.com/c/platform/external/mesa3d/+/761951 (Included in this patch set) https://android-review.googlesource.com/c/platform/external/mesa3d/+/786337 (Included in this patch set) https://android-review.googlesource.com/c/platform/external/mesa3d/+/858424 (Included in this patch set) Besides above changes, we also encounter many building issue, which is related with python module not included. In previous, it will check and use host python module. But with the building enhancement, it is forbidden now. So many file generated during building will report python issue. I have checked the Google source(under external/mesa3d), they pre-generated those files and put them in source code. This will be terrible for us, if we follow Google's method, we have to deal with those files everytime when we do rebase.

ImportError: No module named mako.template ImportError: No module named _elementtree

renchenglei commented 5 years ago

The pre-built change is huge! I will try to sync with Android Guys, and add mako and other tool to Android building system, and check if we could wa the pre-built file issues.

tpalli commented 5 years ago

The pre-built change is huge! I will try to sync with Android Guys, and add mako and other tool to Android building system, and check if we could wa the pre-built file issues.

Agreed, I think it's going to be painful and I don't see the reasons why this is change is being made, it would be good if you can figure out reasons for this.

renchenglei commented 5 years ago

Thanks @tpalli. As suggested by you, maybe 'binary driver' is a good idea for us. In fact, I just synced with AOSP guy, and he also accepted this. I will figure out how to add 'binary driver' to the tree. :)

tpalli commented 5 years ago

Thanks @tpalli. As suggested by you, maybe 'binary driver' is a good idea for us. In fact, I just synced with AOSP guy, and he also accepted this. I will figure out how to add 'binary driver' to the tree. :)

Please also ask about the reasoning here. I saw 'non-hermetic tools' mentioned in one of the commits, which does not seem to make sense. Ideally the driver should build as is. It's good for Chrome OS so there should not be reason why it's not good for Android. Have they discussed with Chrome folks about these changes?

strassek commented 5 years ago

This will be terrible for us, if we follow Google's method, we have to deal with those files everytime when we do rebase.

Hmm, it does introduce a number of possible issues for both developers and maintainers. i.e. if you want to try out a patch from upstream you will now have to always check if you need to regen these files. That being said, I don't necessarily want to stray too far from aosp, or treat mesa a special case. If there is any possibility of retaining the ability to auto-generate files for at least -eng builds I would prefer that.

@renchenglei For now can you share the steps a maintainer would have to follow in order to generate and commit the intermediate files?

Thanks @tpalli. As suggested by you, maybe 'binary driver' is a good idea for us. In fact, I just synced with AOSP guy, and he also accepted this. I will figure out how to add 'binary driver' to the tree. :)

Do you mean prebuilding the driver and making it available somewhere, and having it just download/install during build? Again, that would make it difficult for developers trying to figure out how to actually make changes.

I saw 'non-hermetic tools' mentioned in one of the commits, which does not seem to make sense.

Maybe Google are doing more static analysis on native code in aosp. I'd like to hear the rationale on this too.

It's good for Chrome OS so there should not be reason why it's not good for Android. Have they discussed with Chrome folks about these changes?

It'll be interesting to see how this gets resolved in ARC++.

tpalli commented 5 years ago

Do you mean prebuilding the driver and making it available somewhere, and having it just download/install during build? Again, that would make it difficult for developers trying to figure out how to actually make changes.

I agree but this would seem the easiest way to keep consistent with the upstream tree. We could also try creating a separate build target that only creates all the generated files and somehow create a Android compatible tree with that, not sure how feasible this is though.

renchenglei commented 5 years ago

Thanks @tpalli and @strassek for the suggestion. It will be much better to generate the library. Then we would have one flag to trigger build with pre-built library(make flashfiles INTEL_PREBUILT=true -j8). For developers, we would follow the build rule as it is now. This change only needs update the generated library and add some line to Android.mk file. The diff patch would be easier to maintain. I will prepare one patch, and check if it could work.

renchenglei commented 5 years ago

Something like this: https://github.com/renchenglei/external-mesa/commit/27146652e5bf575ec30786655c7d39d59b8ecb27 Once we have mesa updates, we only need generate the library by manually, and upload them to the diff patch. If the prebuilt flag is enabled, mesa will use the pre-built library. Other wises, mesa will be built from source code by default!

renchenglei commented 5 years ago

Close this PR, as we have one much better on https://github.com/intel/external-mesa/pull/119