Skip to content

Conversation

@ekpyron
Copy link
Collaborator

@ekpyron ekpyron commented Mar 12, 2018

Came up as part of #3674 when adding new test tools.

@axic
Copy link
Contributor

axic commented Mar 12, 2018

@chfast can you double check this?

@ekpyron ekpyron requested a review from chfast March 12, 2018 14:15
@chfast
Copy link
Contributor

chfast commented Mar 12, 2018

This is quite hackish. I'd recommend following independent changes:

  1. Instead of GLOBing all files and then removing some, try to glob individual dirs, you don't want to add each file manually (recommended).
  2. Add another CMakeLists.txt in test/tools to be included by add_subdirectory(tools). The definition of the fuzzer target should be moved there.

@chriseth
Copy link
Contributor

@chfast's suggestions are always good :)

In the medium term, we should also remove the globs in the other CMake files.

@ekpyron
Copy link
Collaborator Author

ekpyron commented Mar 12, 2018

OK - the previous solution was hackish as well, but this is indeed a good opportunity to clean it up at least somewhat.

Moving the solfuzzer target to a CMakeLists.txt in a subdirectory changes its path in the build directory, though - I hope I caught all uses of solfuzzer in the scripts and adjusted them correctly.

Actually I'd usually prefer listing all files individually in CMakeLists.txt, since any globbing (recursive or not) may result in build failures for example if new files are added and cmake is not run again afterwards - but this is actually a separate discussion and so far globbing seems to be used everywhere.

@ekpyron
Copy link
Collaborator Author

ekpyron commented Mar 12, 2018

squashed

Copy link
Contributor

@chfast chfast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks better now.

I'm also for not using GLOB, but it's usually quite a lot of work to replace GLOBs if they were used. That's not for this PR.

Adding files to a single target also can be split into dirs using target_sources().

@axic
Copy link
Contributor

axic commented Mar 12, 2018

I was wondering if it should be moved out of test/tools/ into tools/, but fuzzing is really just for testing so it is better there.

@chriseth chriseth merged commit 886dc05 into develop Mar 12, 2018
@axic axic deleted the moveTestTools branch March 12, 2018 21:40
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.

5 participants