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

Added cmake support for testing against CVEs #354

Merged
merged 10 commits into from
Jun 22, 2019

Conversation

nmoinvaz
Copy link
Member

@nmoinvaz nmoinvaz commented May 30, 2019

This adds cmake support for running and testing minigzip against the CVE gz archives. It adds a python script that will translate the error code from minigzip into what is acceptable for the test.

test/CVE.py Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@mtl1979
Copy link
Collaborator

mtl1979 commented May 30, 2019

Python is not installed by default on many systems, so this might fail without descriptive error message.

@nmoinvaz
Copy link
Member Author

Sadly cmake test doesn't have a way to say certain exit codes are success or failures. I tried to go with a scripting language that has cross-platform support. It is installed on all of the travis build machines at least, but I have also added a friendly error message when python doesn't exist to the cmake.

@mtl1979
Copy link
Collaborator

mtl1979 commented May 30, 2019

@nmoinvaz Travis build images are in no way standard as they are based on very old Ubuntu release with most of the development related packages added with multiple versions. Windows for example doesn't have Python, Perl or cmake installed by default. Fun thing is that even essential C/C++ development files are not installed by default when installing recent versions of Visual Studio as they are not required for modern applications that only use C#.

@nmoinvaz
Copy link
Member Author

I only brought that up to say that it is covered on the test harnesses. I realize that it is not going to be installed on every machine, just like cmake is installed on every machine, or unix bash scripts aren't available on every machine.

@mtl1979
Copy link
Collaborator

mtl1979 commented May 30, 2019

@nmoinvaz I know... Sometimes descriptive error message is better than trying to make the script in multiple scripting languages just to cover all possible operating systems. When we do proper documentation, we can put all the requirements for every step from configuring to building and finally to running the application.

@nmoinvaz
Copy link
Member Author

@mtl1979 I am wondering if I should have the WITH_CVES switch, since configure script does not have it and seems to automatically build with those tests.

@mtl1979
Copy link
Collaborator

mtl1979 commented May 30, 2019

@nmoinvaz You could change it that it defaults to running the tests if Python is available and default to not running when Python is not installed.

@nmoinvaz
Copy link
Member Author

nmoinvaz commented Jun 4, 2019

Good idea, I have made that change.

@Dead2
Copy link
Member

Dead2 commented Jun 7, 2019

Looks good, have not tested yet.

CMakeLists.txt Outdated Show resolved Hide resolved
test/CVE.py Outdated Show resolved Hide resolved
@nmoinvaz
Copy link
Member Author

I have modified the pull request to use a redirect cmake script similar to #362 instead of using python scripts. It adds the ability to that cmake script to specify SUCCESS_EXIT codes.

@Dead2 Dead2 merged commit 6f6bdcb into zlib-ng:develop Jun 22, 2019
@Dead2
Copy link
Member

Dead2 commented Jun 22, 2019

Thanks :)

@nmoinvaz nmoinvaz deleted the improvements/cmake-add-cves branch September 14, 2020 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants