GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Bug #2524: Addition of CMake build files by elfring · Pull Request #18 · cppcheck-opensource/cppcheck

Conversation

elfring commented Mar 19, 2011

Copy link

Copy Markdown

Contributor

This is my third proposal for the building of the Cppcheck software with the tool "CMake" in this year. I hope that the chances will increase for integration of the suggested adjustments into your source repository.

Would you like to accept this retry?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kimmov and others added 15 commits

January 29, 2011 14:17


          Add back CMake build files.

34926ef


          CMake: Add some missing files.

12c800c


          CMake: Fix link path.

57b0794


          CMake: Update gcc warning flags.


          Deletion of unneeded variables in CMake scripts (bug cppcheck-opensou…

64badd6

…rce#2524)

Some header file names were assigned to CMake variables. Some of them were not needed
to be mentioned explicitly because such dependencies will be automatically determined
by the build system.

Signed-off-by: Markus Elfring <[email protected]>


          Merge of checks for Windows in CMake scripts (bug cppcheck-opensource…

2deae3b

…#2524)

A few checks for the Windows environment were merged to stress their relationship.

Signed-off-by: Markus Elfring <[email protected]>


          Use of CMake module "FindPCRE" (bug cppcheck-opensource#2524)

56105a0

A few build scripts were updated for the use of the module "FindPCRE.cmake"
and corresponding variables so that the software can work with the programming
interface "Perl-compatible regular expressions".

Signed-off-by: Markus Elfring <[email protected]>


          Deletion of path selection "../" for CLI executable in a CMake script…

a175c29

… (bug cppcheck-opensource#2524)

The path selection "../" was deleted for the executable target in the build script
for the command line interface.

File name conflicts can be avoided by proper out-of-source builds.

Signed-off-by: Markus Elfring <[email protected]>


          Support for "QtHelp" in CMake script for the GUI (bug cppcheck-openso…

bfbebf9

…urce#2524)

The activation of Qt's help module was added to the build specification for the
graphical user interface.

Signed-off-by: Markus Elfring <[email protected]>


          Movement of filelister* sources to CLI in CMake scripts (bug cppcheck…

4e0e108

…-opensource#2524, cppcheck-opensource#2445)

A few source files were reorganised because of the bug report "Move FileLister
classes from LIB to CLI".
https://sourceforge.net/apps/trac/cppcheck/ticket/2445

The CMake build scripts were updated for these changes.

Signed-off-by: Markus Elfring <[email protected]>


          Consistent use of Cppcheck's library sources in CMake scripts (bug cp…

396e932

…pcheck-opensource#2524)

Three build scripts used the command "aux_source_directory" while Cppcheck's library build
script used the recommended way of listing the relevant source file names explicitly.
This list of source file names was moved to a separate file which is referenced
in the other scripts now.

Signed-off-by: Markus Elfring <[email protected]>


          Support for "TinyXML" in build files (bug cppcheck-opensource#2524)

1950cfe

Source files were embedded into the Cppcheck project from the class library "TinyXML".
The build specifications were adjusted accordingly.

The TinyXML header file is referenced by a few source files without specification
of a subdirectory now.

Signed-off-by: Markus Elfring <[email protected]>


          Completion of variable assignments in CMake scripts (bug cppcheck-ope…

e1dcda8

…nsource#2524)

Some source file names were added to variable assignments in CMake scripts
to update relevant dependencies. This completes the specification of
file lists which are involved in the building of the Cppcheck software.

Signed-off-by: Markus Elfring <[email protected]>


          Correction for flags setting in CMake scripts (bug cppcheck-opensourc…

6c1e017

…e#2524)

Additional compilation flags were passed to the command "set" as a second parameter.
This had the effect that the passed parameters were converted to a CMake list.
The variable "CMAKE_CXX_FLAGS" got an unwanted semicolon if it was not empty before.

These flags are handled by a single string now.

Signed-off-by: Markus Elfring <[email protected]>


          Use of CMake module "FindXmlTo" (bug cppcheck-opensource#2524)

8e379f6

The documentation build script was updated for the use of the module "FindXmlTo.cmake".

Signed-off-by: Markus Elfring <[email protected]>

danmar commented Mar 20, 2011

Copy link

Copy Markdown

Collaborator

You still include PCRE / TinyXML in the build.

You don't include the filelister_unix.cpp in the win32 build, and you don't include filelister_win32.cpp in unix builds ... can you explain why? Separating the configurations is ok if it's necessary but when there is no need I just think it's making the maintenance harder.

So.. I don't want to merge this neither.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danmar commented Mar 20, 2011

Copy link

Copy Markdown

Collaborator

I removed filelister_unix and filelister_win32 with:
http://github.com/danmar/cppcheck/commit/8caf96be63f08079daa66078dc502d29a452ffed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

elfring commented Mar 20, 2011

Copy link

Copy Markdown

Contributor Author

Yes. - The CMake specification contains also checks for the build environment in a corresponding way like they are used in your source files for conditional compilation.

I am curious if you plan further adjustments for a class like "FileLister". How would you like to continue software development for this component (including build files) now?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kimmov commented Mar 20, 2011

Copy link

Copy Markdown

Contributor

Based on quick very cursory look some of the comments were addressed. But we are still getting something we don't want. We wanted the bicycle but you are offering the car. This is better but not improved enough.

Just for an example - 396e932 is not needed at this point. And its commit message is broken - watch for line lengths.

Why you keep pushing patches we have said each time we don't want now? Don't you just understand English?

So from my point of view this set is vetoed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

elfring commented Mar 20, 2011

Copy link

Copy Markdown

Contributor Author

I have got the impression that I offer you a robust "mountain bike".

The clean-up from commit 396e932 is needed for a proper build.

Would you like to provide a patch series for this CMake issue which you find more acceptable from your point of view?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kimmov commented Mar 20, 2011

Copy link

Copy Markdown

Contributor

As I've told earlier I'm not going to touch CMake build system anymore. I wasted way too much time with it earlier. But obviously I have to make some updated later on if it gets accepted as a new build system. And therefore I want to make sure the system is good one and done with proper steps and good quality. I'm not going to start fixing things.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kimmov commented Mar 20, 2011

Copy link

Copy Markdown

Contributor

And we don't need your "proper build" (whatever it is). There is no "proper build", there is build that works for all configurations and platforms and is maintainable. None of these goals require your cleanup patches, some of which are just your own personal style preference.

Cleanup should happen later on once we have the working system accepted and included. That one developer gets it working does not mean it works for everybody and for every platform. Reminds me my earlier attempt with CMake... Worked for many people but apparently not for everybody and so it got removed behind my back instead of giving me any hint there is something broken and let me try to fix it. I'd rather try to avoid that this time.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

elfring commented Mar 20, 2011

Copy link

Copy Markdown

Contributor Author

I find that your previous experience indicates communication problems. I hope that we can avoid them here.

I would expect that my CMake suggestion will mostly work.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danmar commented Mar 23, 2011

Copy link

Copy Markdown

Collaborator

I won't pull this neither. It's pretty good but it is not what we asked for.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danmar closed this

Mar 23, 2011

seb777 commented Mar 23, 2011

Copy link

Copy Markdown

Collaborator

It's a shame that this contribution was not accepted.
Cppcheck has already project files for Microsoft Visual Studio, Code Blocks and maybe others. Why not for CMake ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kimmov commented Mar 23, 2011

Copy link

Copy Markdown

Contributor

Please read the previous discussions first before starting new discussion. That sounded more like trolling now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

kimmov commented Mar 23, 2011

Copy link

Copy Markdown

Contributor

And to summarize:

nobody is against CMake - I think it is welcome addition and will help Cppcheck building - after all it is based on my earlier work and ideas
but adding new build system is not something that can be done without lots of testing and review. A lot more than for "normal" patches. It simply must work.
I really really don't want it be removed again - if it gets added it means it will be used too - which requires it really works for everybody
I don't want to see "initial work" committed because "it works" and then being fixed "really working this time" when people try to use it. Leading people not wanting to use because it didn't work when they tried it.

And I think we should not pull the CMake directly to master branch anyway. Few days or couple of weeks in separate branch for people to test would probably help finding the most annoying bugs / annoyances.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

seb777 commented Mar 23, 2011

Copy link

Copy Markdown

Collaborator

And to conclude:

What is proposed is:

Elfring should make a fork and integrates its commits on that branch
Then he waits for any bugs and ideas of improvement from users ( the most annoying bugs / annoyances)
After an incubation time, Kimmov merges the Elfring's branch on the master branch

Is that correct ?

IMHO, It seems to be a good compromise.
Anyone interested in the Elfring's work can recover to his repository.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

elfring commented Mar 23, 2011

Copy link

Copy Markdown

Contributor Author

It's pretty good but it is not what we asked for.

@ Daniel:
Which implementation details from my CMake approach do not fit to your expectations at the moment?

All reactions

Sorry, something went wrong.