Zstd: User-defined SHARED_LIBRARY_OUTPUT_NAME

[aparamon]

Currently, CMake build sets up SHARED_LIBRARY_OUTPUT_NAME based on LIBRARY_BASE_NAME, LIBVER_* and architecture. It is proposed that user could override SHARED_LIBRARY_OUTPUT_NAME with her setting, allowing to build shared library as e.g. "zstdlib.dll".

[Cyan4973]

Sounds like a reasonable feature request to me.
My understanding of cmake is limited, I would gladly accept a PR for this feature.

[aparamon]

So is mine :-) but I think the attached patch does the job.

SHARED_LIBRARY_OUTPUT_NAME.txt

[Cyan4973]

Thanks ! Your patch has been included in cmake script

[aparamon]

It is broken again in the latest release :-(
Seemingly, after commit https://github.com/facebook/zstd/commit/e87cad1053e5a50ce280bd346b016fe294b64605

[terrelln]

We test the cmake build script in the CI, but since it was broken we aren't testing this. Is there a way we can test this functionality in CI so it doesn't break again?

[Cyan4973]

Indeed, since cmake script evolves from external requests and patch, overlapping or even conflicting objectives may fight to get in.

A few thoughts :

• When adding a feature, provide enough comments so that an external contributor understands what it is about, thus reducing the odd of the feature being unintentionally erased, or deemed "unimportant" to preserve.
• To ensure a feature remain active in the future, provide both the patch _and associated test_ that will be played in CI. In general, external contributors won't fight a broken test, and will simply try to keep it running properly.

[aparamon]

Hmm, are there automated tests for CMake options already in the CI? I presume their place would be either
https://github.com/facebook/zstd/blob/dev/.travis.yml or https://github.com/facebook/zstd/blob/dev/appveyor.yml ?

[Cyan4973]

Yes :
For travis : https://github.com/facebook/zstd/blob/master/.travis.yml#L16
note : the tests in master are run on a nightly basis, instead of "at every push/modif"

For appveyor : https://github.com/facebook/zstd/blob/dev/appveyor.yml#L26
here also, run only on master

This policy can be changed, as need be.
We have a difficult balance to preserve between test completeness and feedback signal speed.

[aparamon]

I fail to see the actual checks. Is it supposed that I add additional build rule, for which I specify some non-default shared library name (e.g. "zstdlib"), and a "test -f" check?

[Cyan4973]

The point is more for contributors to add a test, typically in script format, that will check that a specific property is confirmed (in this case, that user can take control of SHARED_LIBRARY_OUTPUT_NAME).

We can take care of the part where this test is run automatically in CI.