Skip to content

Fix build settings #357

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

Merged
merged 3 commits into from
Oct 9, 2014
Merged

Fix build settings #357

merged 3 commits into from
Oct 9, 2014

Conversation

awiddersheim
Copy link
Member

No description provided.

@@ -263,7 +263,9 @@ endif

.PHONY: build
build: ${TARGET}
ifneq (${TARGET},failtarget)
${MAKE} settings
Copy link
Member

Choose a reason for hiding this comment

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

Going recessive on makefiles is general what we are trying to get away from.

if you need to do this you need to the build: ${TARGET} to build: settings using ifndef.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure what you mean. Sorry. Can you try explaining differently or using a code example of how this should be done?

Copy link
Member

Choose a reason for hiding this comment

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

Should be done like this:

ifneq (${TARGET},failtarget)
build: ${TARGET}
else 
build: failtarget 
endif 

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how that is going to fix the problem I am trying to solve. Perhaps I should explain the issue more. Right now if I just do make with no TARGET specified, the help is displayed as well as the settings. I don't really find that necessary. This commit prevents the settings from being displayed in that occurrence.

Copy link
Member

Choose a reason for hiding this comment

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

i will just shut my cake hole now ;)

Yeah i did not get it on a few levels.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha. OK. No problem. Hopefully, things are more clear now? I'm open to making things better and taking feedback. It is no problem.

@jrossi
Copy link
Member

jrossi commented Oct 8, 2014

Personal input and nothing more:

The make files are not for users they are for developers. I use it to verify the config and settings are what I expect when doing different things. Also makes sense for travis-ci where output is recorded for review later.

Default for users should always be less OUTPUT unless something bad happens. And another point WE ARE not users what were like are different then what users like :) i think ;)

@awiddersheim
Copy link
Member Author

This makes sense. I never read this before or I just merely glanced over your thinking from the other pull request. I think in general that logic is fine.

If you are in opposition of removing the settings output than I'm not opposed. I made the pull request merely as a suggestion but no hard feelings if it is removed.

#346

Perhaps I am still misunderstanding something.

@jrossi
Copy link
Member

jrossi commented Oct 8, 2014

no feelings ;) i just complete miss read what you were doing on this pull request. This is 100% my fault ;)

@awiddersheim
Copy link
Member Author

OK. No worries.

@jrossi
Copy link
Member

jrossi commented Oct 8, 2014

I should make it clear I don't care enough to be in opposition of anything that goes on with settings. I mean if people like it will stay. They don't it will go, but never by my hand :)

I might push to make the build visually clean to users so that when it outputs errors it feels like a bug. Less griberish on the screen for users makes the software seam more complete and finished. Redis is my example of this. People open up bug reports for warning messages and I think that is great.

@awiddersheim
Copy link
Member Author

Thanks for clarifying. Lets get this merged?

@cgzones
Copy link
Contributor

cgzones commented Oct 9, 2014

Maybe you can extend the condition until the end of build, cause the green output Done building failtarget is a little bit irritating?

Per the suggestion of cgzones in ossec#357, the if statement has been
extended so that the completion message is not displayed when doing a
build for 'failtarget'.
@awiddersheim
Copy link
Member Author

@cgzones I update the pull request with the suggested changes.

cgzones added a commit that referenced this pull request Oct 9, 2014
@cgzones cgzones merged commit 76db39f into ossec:master Oct 9, 2014
@awiddersheim awiddersheim deleted the fix_build_settings branch October 9, 2014 14:27
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.

3 participants