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

Document the generation of static files #814

Closed
kelson42 opened this issue Sep 6, 2022 · 11 comments · Fixed by #816
Closed

Document the generation of static files #814

kelson42 opened this issue Sep 6, 2022 · 11 comments · Fixed by #816
Assignees
Milestone

Comments

@kelson42
Copy link
Collaborator

kelson42 commented Sep 6, 2022

The way it work and what is expected from devs is not obvious. See #813

@kelson42 kelson42 added this to the 12.1.0 milestone Sep 6, 2022
@kelson42 kelson42 self-assigned this Sep 6, 2022
@mgautierfr
Copy link
Member

I'm commenting your comment here

The feature of git is quite clear but not all users are familiar with this.

We are documenting libzim usage. We will not document git usage or gcc, or cpp.
Lot of features of softwares/technologies are mandatory prerequisite to develop libzim (or any other software).
We already have documentation to how install ninja or meson. We have kiwix-build compiling everything. We will not start to document git usage

More problematic is that files which have to be commited and pushed in many scenarios are put in .gitignore.

Which files ?
No files in build have to be pushed on git repository. This is why build is in .gitignore.
If it is the case, it is a bug and we must adapt .gitignore. Not adding new documentation.

@kelson42
Copy link
Collaborator Author

kelson42 commented Sep 7, 2022

The problem is that the cacheid has to be updated and if you don't know about it you won't commit it. Actually in previous PR, it seems that only the test server has been updated with a new cacheid. Seems wrong to me.

@mgautierfr
Copy link
Member

It is not related to git at all then.

The cacheid is simply the first eight char of the sha1 sum.
It can be computed with sha1sum tool on the generated resources (in build directory if you compile yourself libkiwix. In BUILD_native_dyn/libkiwix if you use kiwix-build)
Or even better, just adapt the test with the new value computed (and displayed in error message) when tests are failing.
[This paragraphe is probably what we have to put in the README in the correct section]

Git is no related to that, you don't have to commit generated file

Actually in previous PR, it seems that only the test server has been updated with a new cacheid. Seems wrong to me.

How it seems wrong ? "Everything" is automatized. You don't have to compute anything to have libkiwix working.
The only thing which is not are the tests as they are testing that automatized things are working. Automatizing tests would open the door to using buggy value both in the output and in the test.

@kelson42
Copy link
Collaborator Author

kelson42 commented Sep 7, 2022

How it seems wrong ?

If the cacheid is generated at the compilation and should not be part of the source, then why it is part of test/server.cpp?

@mgautierfr
Copy link
Member

Because it is part of the final files served by the server. And test/server.cpp is testing that server return the correct data.

@kelson42
Copy link
Collaborator Author

kelson42 commented Sep 7, 2022

So, why test/server.cpp does not appear to have to be commited?

@mgautierfr
Copy link
Member

Because you have to modify it and git add it.
As I've said : The only thing which is not are the tests as they are testing that automatized things are working. Automatizing tests would open the door to using buggy value both in the output and in the test.

@kelson42
Copy link
Collaborator Author

kelson42 commented Sep 7, 2022

After this discussion, this is more clear to me what has to be documented. I just want to remind that if we would generate all the time a random cacheid, we would have avoid a bit of cmplexity and all this discussion. I keep thinking that current approach is over-enginered.

@kelson42
Copy link
Collaborator Author

@juuz0 @veloman-yunkan @mgautierfr So, what are exactly the instructions to run to update the test/server.cpp if one of the static ressource is changed?

@juuz0
Copy link
Collaborator

juuz0 commented Sep 12, 2022

@kelson42 How I currently do this:

  1. run tests (cd build && meson test)
  2. after it fails, go to testlog file.
  3. copy value from there

@kelson42
Copy link
Collaborator Author

kelson42 commented Sep 12, 2022

@juuz0 Thank you for your feedback. It makes sense to do like that indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants