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

[doc] Revise howto #3222

Merged
merged 11 commits into from
Mar 19, 2021
Merged

[doc] Revise howto #3222

merged 11 commits into from
Mar 19, 2021

Conversation

zomen2
Copy link
Contributor

@zomen2 zomen2 commented Mar 11, 2021

No description provided.

@zomen2 zomen2 added WIP 💣 Work In Progress documentation 📖 Changes to documentation. labels Mar 11, 2021
@@ -0,0 +1,34 @@
project_root_dir := $(strip $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To start a make process from other directory is allowed. This line prevents accidentally deletions in this case by clean target.

Copy link
Contributor

Choose a reason for hiding this comment

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

This Makefile is really complex and it contains a lot of unnecessary things. This is just a test project, so please keep it as simple as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is not subject of the howto document. It only works and nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the subject of this patch. Later if someone wants to modify this file it will be hard to understand it, because it is very complex. So I still recommend to simplify this. The whole Makefile can look like this:

CFLAGS = -Iincl

all:
	$(CC) $(CFLAGS) -c src/divide.c -o /dev/null
	$(CC) $(CFLAGS) -c src/main.c -o /dev/null

Copy link
Contributor

Choose a reason for hiding this comment

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

This is marked as resolved but it is not resolved yet. Please do not mark comment as resolved untill it is trully resolved or give me a reason why it is not resolved.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @csordasmarton to simplify the makefile to the simple suggested one. At the end of the day it is only consists of 3 trivial files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • As you mentioned: "Later if someone wants to modify this file...". Why would somebody want to change the makefile? Because it changed the hierarchy of this "project". Your suggestion requires makefile modification too if some file is added or removed. Mine is resistant for that.

  • In the howto document sometimes we clean the project and re-make it. Clean feature is not supported by your suggested solution.

Our purpose with that howto is to inspire users to play with it. There are some other issues in these C sources. If the user tries to repair them then he should re-make the project. But your suggested solution will not make if the user changes the divide.h file.

  • The make can be called not only from that directory where the makefile resides. For example:

cd && make -f docs/examples/Makefile
should work. But your suggested makefile will not.

I think I have explained the requirements that were in my mind while creating that makefile.

I think if these requirements are valid then the "simplified" makefile will be at least comlex as mine is.

So, please specify your requirements against the makefile. And I will simplify it following them.

Copy link
Contributor

Choose a reason for hiding this comment

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

With my solution there will be no generated files (output is redirected to /dev/null) so there is no need for clean target.

You don't have to step into the directory where the Makefile can be found, you can use the -C option of the make command for it if you would like to:

$ make -C docs/examples/
make: Entering directory '~/codechecker/docs/examples'
cc -Iincl -c src/divide.c -o /dev/null
cc -Iincl -c src/main.c -o /dev/null
make: Leaving directory '~/codechecker/docs/examples'

I still not recommend this complex Makefile for this simple example project but I will not block this PR. So on my side I finished the review of this patch.

docs/examples/incl/ArgParser.hpp Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
@csordasmarton csordasmarton added this to the release 6.16.0 milestone Mar 11, 2021
@csordasmarton
Copy link
Contributor

@zomen2 When you create a commit the window where you type the commit message will describe the recommended commit format. Please use the recommended format for commits. You can find the commit format also in our repository: https://github.com/Ericsson/codechecker/blob/master/.gitmessage.

Also add some description to the commit, what do you do in your patch.

Copy link
Contributor

@csordasmarton csordasmarton left a comment

Choose a reason for hiding this comment

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

Please try to keep everything as simple as possible and do not complicate things.

@@ -0,0 +1,34 @@
project_root_dir := $(strip $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

This Makefile is really complex and it contains a lot of unnecessary things. This is just a test project, so please keep it as simple as possible.

docs/usage.md Outdated Show resolved Hide resolved
Howto follow modifications happened on command line user interface.
@zomen2 zomen2 changed the title Revise howto [doc] Revise howto Mar 12, 2021
zomen2 added 2 commits March 12, 2021 19:16
Howto follow modifications happened on command line user interface.
docs/examples/Makefile Outdated Show resolved Hide resolved
docs/usage.md Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

Some typos and some suggested extensions

docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
dkrupp
dkrupp previously requested changes Mar 16, 2021
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

please look ateh the simplified c example

docs/examples/src/main.cpp Outdated Show resolved Hide resolved
Howto follow modifications happened on command line user interface.
docs/usage.md Outdated Show resolved Hide resolved
@zomen2 zomen2 removed the WIP 💣 Work In Progress label Mar 17, 2021
@zomen2 zomen2 requested review from csordasmarton and dkrupp March 17, 2021 16:12
Howto follow modifications happened on command line user interface.
Copy link
Contributor

@csordasmarton csordasmarton left a comment

Choose a reason for hiding this comment

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

One import thing is to not use the --enable-all option for the CodeChecker analyze/check commands because it is not recommended. It will generate to much noise/reports. So please remove this option from this documentation too. Enable specific checkers with the --enable option.

@@ -0,0 +1,34 @@
project_root_dir := $(strip $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the subject of this patch. Later if someone wants to modify this file it will be hard to understand it, because it is very complex. So I still recommend to simplify this. The whole Makefile can look like this:

CFLAGS = -Iincl

all:
	$(CC) $(CFLAGS) -c src/divide.c -o /dev/null
	$(CC) $(CFLAGS) -c src/main.c -o /dev/null

docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
zomen2 added 3 commits March 18, 2021 15:55
Howto follow modifications happened on command line user interface.
Howto follow modifications happened on command line user interface.
Howto follow modifications happened on command line user interface.
@zomen2 zomen2 requested a review from csordasmarton March 18, 2021 18:49
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
@@ -0,0 +1,34 @@
project_root_dir := $(strip $(shell dirname $(realpath $(lastword $(MAKEFILE_LIST)))))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is marked as resolved but it is not resolved yet. Please do not mark comment as resolved untill it is trully resolved or give me a reason why it is not resolved.

docs/usage.md Show resolved Hide resolved
docs/usage.md Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
Howto follow modifications happened on command line user interface.
@zomen2 zomen2 requested a review from csordasmarton March 19, 2021 11:33
@csordasmarton csordasmarton dismissed dkrupp’s stale review March 19, 2021 14:15

Comments are fixed.

@csordasmarton csordasmarton merged commit 1eaf280 into Ericsson:master Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📖 Changes to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants