libzim icon indicating copy to clipboard operation
libzim copied to clipboard

Remove 'subprojects' dir

Open kelson42 opened this issue 5 years ago • 8 comments

kelson42 avatar Oct 29 '20 08:10 kelson42

Codecov Report

Merging #441 into master will decrease coverage by 10.77%. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #441       +/-   ##
===========================================
- Coverage   59.09%   48.31%   -10.78%     
===========================================
  Files          63       75       +12     
  Lines        2743     3355      +612     
  Branches     1171     1468      +297     
===========================================
  Hits         1621     1621               
- Misses       1120     1732      +612     
  Partials        2        2               
Impacted Files Coverage Δ
include/zim/writer/url.h 56.25% <0.00%> (-43.75%) :arrow_down:
src/writer/_dirent.h 67.74% <0.00%> (-32.26%) :arrow_down:
src/writer/cluster.h 50.00% <0.00%> (-13.64%) :arrow_down:
src/writer/workers.h 0.00% <0.00%> (ø)
src/writer/workers.cpp 0.00% <0.00%> (ø)
include/zim/writer/creator.h 0.00% <0.00%> (ø)
src/writer/queue.h 0.00% <0.00%> (ø)
src/writer/article.cpp 0.00% <0.00%> (ø)
src/tools.cpp 0.00% <0.00%> (ø)
src/writer/creatordata.h 0.00% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0d8c23a...d49cd32. Read the comment docs.

codecov[bot] avatar Oct 29 '20 08:10 codecov[bot]

I think the code coverage drop is an artefact of Codecov.

kelson42 avatar Oct 29 '20 08:10 kelson42

@mgautierfr Who/what needs that? gtest is properly packaged meanwhile. It seems that if gtest is not available it uses it in place of asking to install the package.

kelson42 avatar Oct 29 '20 08:10 kelson42

Who/what needs that?

Developers. Maybe the CI.

Meson has the ability to build the dependencies of a project itself. We are a bit far from the ideal situation, but in a perfect meson world, building kiwix-tools would be a simple meson build . && cd build && ninja No dependency to download and compile (from zlib to kiwix-lib), Meson would care of that, whatever is your system (windows, exotic linux distribution, ...). On top of that, it ensure that dependency are build using the same compilation options than the main project. In this meson world, kiwix-build would be useless.

It doesn't means that meson will download the dependency. If it found one already present, it will use it.

While we are not able to build everything this way, I think we should go in this direction. And it is potentially the way to have the libzim CI working on windows' github actions.

mgautierfr avatar Oct 29 '20 09:10 mgautierfr

  • The CI does not need it, everything is green here.
  • The developers should not need it because it is packaged and otherwise they can install manually easily.
  • I don't want to have any compilation code which compiles anything else than the libzim itself, because this is not the purpose of this repository. It has been put here in a first place because we had a problem with getting gtest dependence. This reason has vanished.
  • I don't like to have us maintaining things which are not stricly necessary to deliver the value of the repository. Actually here it was not even maintained, the gtest used is pretty old.
  • I don't like the idea to somehow privilege a specific version of deps or way they should be compiled. The source code should be portable and robust to many deps version or compilation ways. This is why we have a CI with a large variety of environment and compilation styles.

I understand what you say about Meson, and this is great if the promise is delivered, but this is an added value for kiwix-build, not here.

kelson42 avatar Oct 29 '20 10:10 kelson42

The CI does not need it, everything is green here.

The CI doesn't need it yes. But it does means we should remove it. In fact, we could have the CI independent of kiwix-build and it would be better

The developers should not need it because it is packaged and otherwise they can install manually easily.

Yes. And what ever the wrap file is here or not, they still can do.

I don't want to have any compilation code which compiles anything else than the libzim itself, because this is not the purpose of this repository.

If you don't want, install the dependency first and meson will not compile dependencies. Or pass the option --wrap-mode=nofallback to avoid it. It is what is made when distribution packages projects and it works pretty well.

I don't like to have us maintaining things which are not stricly necessary to deliver the value of the repository. Actually here it was not even maintained, the gtest used is pretty old.

There is not much to maintain. Just follow the version update. We still have to do it in kiwix-build and the CI. I think it is better to have the dependency definition in the project using it than in another repository. And update the version is just running meson subprojects update times to times. The wrap file is not something we maintain, it comes from https://wrapdb.mesonbuild.com/

I don't like the idea to somehow privilege a specific version of deps or way they should be compiled. The source code should be portable and robust to many deps version or compilation ways. This is why we have a CI with a large variety of environment and compilation styles.

This is not. Having the right dependencies is a complex thing. Most of the CI install a fixed version of dependencies and most of the "build" systems (python/node/rust/...) have a (recommended) way to fix specific version when delivering project. It doesn't force you to use a specific version.

I understand what you say about Meson, and this is great if the promise is delivered, but this is an added value for kiwix-build, not here.

kiwix-build is a added value for libzim. It allow the user to simply have all the dependencies compiled. Meson can do the same, without asking the user to download kiwix-build and without interfering with kiwix-build neither. We cannot use this meson feature without putting it in libzim.

Instead of removing the gtest.wrap file I think we should add the wrap file for the other dependencies (zstd, zlib, lzma). It would allow user to download and compile everything needed for libzim, on all platform without any need for other tools than meson/ninja.

mgautierfr avatar Oct 29 '20 10:10 mgautierfr

The wrap file is not something we maintain, it comes from https://wrapdb.mesonbuild.com/

Why it is in the repo then? Why not downloading it at the runtime if needed?

kelson42 avatar Oct 29 '20 10:10 kelson42

This is how meson works. Ask them, not me :)

But I will answer with I think is the answer. The wrap file just tell meson how to download the source code of the project and have a subproject directory ready to be compiled using meson. We could :

  • Simply have this directory (plain copy, git submodule, copy at runtime, ...) and no wrap file
  • Create and maintain/patch our own wrap file for a specific dependency
  • Use "well maintained" wrap file for common project. (and meson provide tools to do it)

Either way, we have to tell meson what to do and this file is the way to tell it.

Why not downloading it at the runtime if needed?

We would need a way to detect we need it. And a way to know what we need to download. It would be just another step to download a file telling what to download.

mgautierfr avatar Oct 29 '20 11:10 mgautierfr