Issues in the submitted `3.0.2+1` package

[boris-kolpackov]

Noticed a couple of issues in the submitted 3.0.2+1 package:

1. There is some strange/unnecessary raw HTML markup in PACKAGE-README.md. FYI, it is ignored in the package page on cppget.org: https://queue.cppget.org/libminiz/3.0.2+1

2. You normally don't want exe{*}: test = true in the main project (only in tests/examples subprojects). In this case, since there is only library being built, it's harmless but useless.

3. The tests/build/root.build is quite strange. Firstly, if you need both c and cxx, you should load those individual modules rather than loading cc (it has the same effect but it's not meant to be used this way). Secondly, it's unclear why you need the cxx module here since all the tests appear to be in C. Finally, I don't think any of the tests are executed for the test operation because you have exe{*}: test = true commented out.

I am going to hold off on publishing the package mainly because of the last item, which seems pretty broken.

[lyrahgames]

1. There is some strange/unnecessary raw HTML markup in PACKAGE-README.md. FYI, it is ignored in the package page on cppget.org: https://queue.cppget.org/libminiz/3.0.2+1

Yes, I already recognized this, but I use the HTML markup for the GitHub Repo to show some badges and to center align the first part of the README.md. I use this kind of HTML code for several build2 packages for development overview and status information. I find it quite good that it is ignored on the package page as this information is not meant for consumption.

2. You normally don't want exe{*}: test = true in the main project (only in tests/examples subprojects). In this case, since there is only library being built, it's harmless but useless.

Thanks for noticing. That is probably a copy-and-paste mistake. I will fix it for the next release.

3. The tests/build/root.build is quite strange. Firstly, if you need both c and cxx, you should load those individual modules rather than loading cc (it has the same effect but it's not meant to be used this way).

Good to know. Then I will restrain myself from using the cc module. Thought that I would need to load cc if I would like to use C and C++ at the same time.

Secondly, it's unclear why you need the cxx module here since all the tests appear to be in C.

You are right. Currently, all tests that are compiled are only based on C. But the original tests also listed C++ files that I tried to compile and execute. Sadly, the original build system did not provide rules for testing. I removed my trial in the buildfile but forgot to remove the symbolic links and to update the root.build. I will fix this for the next release.

Finally, I don't think any of the tests are executed for the test operation because you have exe{*}: test = true commented out.

Actually, this is on purpose. In the README.md under section Issues and Notes, I put the remark The test fuzzers are only built and not executed, as they use something like oss-fuzz. As I cannot properly run the tests, I at least wanted to make sure that they would compile. I should have also remarked this in the root.build and/or buildfile.

I am going to hold off on publishing the package mainly because of the last item, which seems pretty broken.

In this sense, it is not really broken and you do not need to hold it off. But I can also address these issues first and publish a new revision.

[boris-kolpackov]

Thanks for the clarifications.

In this sense, it is not really broken and you do not need to hold it off. But I can also address these issues first and publish a new revision.

While true, loading the C++ module in a C-only library is not ideal (for example, the build will fail if there is C but not C++ compiler). So I think it's better to fix this with a revision.

[lyrahgames]

Thanks for the clarifications.

You are very welcome ^^

While true, loading the C++ module in a C-only library is not ideal (for example, the build will fail if there is C but not C++ compiler).

Didn't think of that. Good to know. Thanks.

So I think it's better to fix this with a revision.

No problem. Fixed all the issues in the last few commits and already released and published a new revision. For the sake of completeness, here is the up-to-date CI run. I will close this issue as resolved but feel free to reopen it.

[boris-kolpackov]

@lyrahgames

FYI, you still left stray cxx.std = latest: https://github.com/build2-packaging/miniz/blob/master/libminiz/tests/build/root.build#L1

[lyrahgames]

Sorry about that. I did the changes from before on a train ride and must have been a little bit inattentive then. Hopefully, everything is fixed now.