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

Make fish build more quietly. #3248

Merged
merged 2 commits into from Aug 2, 2016
Merged

Make fish build more quietly. #3248

merged 2 commits into from Aug 2, 2016

Conversation

floam
Copy link
Member

@floam floam commented Jul 16, 2016

Description

I made make a lot quieter and want to get feedback. It needs to @echo a few more things than it does now and it makes travis mad. It also currently lacks a way to flip it off aside from commenting out the added definition at the top of Makefile.in. If people generally are cool with it, that should all be pretty trivial.

I think we should be more likely to notice warning output now, with a better SNR.

Output

Here's the new output (hit triangle).

FISH_BUILD_VERSION = 2.3.1-393-g819cf26
clang++ [src/complete.cpp]
In file included from src/complete.cpp:35:
./src/parser.h:274:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
    block_t *const current_block();
             ^~~~~~
1 warning generated.
clang++ [src/event.cpp]
In file included from src/event.cpp:16:
./src/parser.h:274:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
    block_t *const current_block();
             ^~~~~~
1 warning generated.
clang++ [src/exec.cpp]
In file included from src/exec.cpp:37:
./src/parser.h:274:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
    block_t *const current_block();
             ^~~~~~
1 warning generated.
clang++ [src/expand.cpp]
clang++ [src/fallback.cpp]
clang++ [src/fish_version.cpp]
clang++ [src/function.cpp]
clang++ [src/highlight.cpp]
clang++ [src/history.cpp]
clang++ [src/input.cpp]
In file included from src/input.cpp:33:
./src/parser.h:274:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
    block_t *const current_block();
             ^~~~~~
1 warning generated.
clang++ [src/input_common.cpp]
clang++ [src/intern.cpp]
clang++ [src/io.cpp]
clang++ [src/iothread.cpp]
clang++ [src/kill.cpp]
clang++ [src/output.cpp]
clang++ [src/pager.cpp]
clang++ [src/parse_execution.cpp]
In file included from src/parse_execution.cpp:39:
./src/parser.h:274:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
    block_t *const current_block();
             ^~~~~~
1 warning generated.
clang++ [src/parse_productions.cpp]
clang++ [src/parse_tree.cpp]
clang++ [src/parse_util.cpp]
clang++ [src/parser.cpp]
In file included from src/parser.cpp:21:
./src/parser.h:274:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
    block_t *const current_block();
             ^~~~~~
src/parser.cpp:258:10: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
block_t *const parser_t::current_block() { return block_stack.empty() ? NULL : block_stack.back(); }
         ^~~~~~
2 warnings generated.
clang++ [src/parser_keywords.cpp]
clang++ [src/path.cpp]
clang++ [src/postfork.cpp]
clang++ [src/proc.cpp]
In file included from src/proc.cpp:41:
./src/parser.h:274:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
    block_t *const current_block();
             ^~~~~~
1 warning generated.
clang++ [src/reader.cpp]
In file included from src/reader.cpp:64:
./src/parser.h:274:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
    block_t *const current_block();
             ^~~~~~
1 warning generated.
clang++ [src/sanity.cpp]
clang++ [src/screen.cpp]
clang++ [src/signal.cpp]
clang++ [src/tokenizer.cpp]
clang++ [src/utf8.cpp]
clang++ [src/util.cpp]
clang++ [src/wcstringutil.cpp]
clang++ [src/wgetopt.cpp]
clang++ [src/wildcard.cpp]
clang++ [src/wutil.cpp]

Compiling fish with CXXFLAGS=-g -O2 -Wignored-qualifiers -fno-exceptions -Wall -Wno-sign-compare -I/usr/local/Cellar/pcre2/10.21/include -iquote. -iquote./src/ -DLOCALEDIR="/usr/local/share/locale" -DPREFIX=L"/usr/local" -DDATADIR=L"/usr/local/share" -DSYSCONFDIR=L"/usr/local/etc" -DBINDIR=L"/usr/local/bin" -DDOCDIR=L"/usr/local/share/doc/fish" -DFISH_BUILD_VERSION="2.3.1-393-g819cf26"
===
clang++ [src/fish_indent.cpp]
clang++ [src/print_help.cpp]

Compiling fish_indent with CXXFLAGS=-g -O2 -Wignored-qualifiers -fno-exceptions -Wall -Wno-sign-compare -I/usr/local/Cellar/pcre2/10.21/include -iquote. -iquote./src/ -DLOCALEDIR="/usr/local/share/locale" -DPREFIX=L"/usr/local" -DDATADIR=L"/usr/local/share" -DSYSCONFDIR=L"/usr/local/etc" -DBINDIR=L"/usr/local/bin" -DDOCDIR=L"/usr/local/share/doc/fish" -DFISH_BUILD_VERSION="2.3.1-393-g819cf26"

clang++ [src/fish_key_reader.cpp]
src/fish_key_reader.cpp:55:14: warning: 'const' type qualifier on return type has no effect [-Wignored-qualifiers]
static char *const sequence_name(wchar_t wc) {
             ^~~~~~
1 warning generated.

Compiling fish_key_reader with CXXFLAGS=-g -O2 -Wignored-qualifiers -fno-exceptions -Wall -Wno-sign-compare -I/usr/local/Cellar/pcre2/10.21/include -iquote. -iquote./src/ -DLOCALEDIR="/usr/local/share/locale" -DPREFIX=L"/usr/local" -DDATADIR=L"/usr/local/share" -DSYSCONFDIR=L"/usr/local/etc" -DBINDIR=L"/usr/local/bin" -DDOCDIR=L"/usr/local/share/doc/fish" -DFISH_BUILD_VERSION="2.3.1-393-g819cf26"
/Users/floam/src/githubfish/doc.h:5646: warning: found </pre> tag without matching <pre>
/Users/floam/src/githubfish/doc.h:5646: warning: unexpected command endhtmlonly
Lexicon filter is not available. Continuing without.
fish has now been built.
Use '/Applications/Xcode-beta.app/Contents/Developer/usr/bin/make install' to install fish.
install: share/tools/web_config/js: Inappropriate file type or format
install: share/tools/web_config/partials: Inappropriate file type or format
install: share/tools/web_config/sample_prompts: Inappropriate file type or format
fish is now installed on your system.
To run fish, type 'fish' in your terminal.

To use fish as your login shell:
* use the command 'chsh -s /usr/local/bin/fish'.

To set your colors, run 'fish_config'
To scan your man pages for completions, run 'fish_update_completions'
To autocomplete command suggestions press Ctrl + F or right arrow key.

Have fun!
(I added `-Wignored-qualifiers` to just make it a little more noisy as fish currently builds with 0 warnings.)

@floam floam force-pushed the shutupmake branch 2 times, most recently from 4d2c46e to 19a15a4 Compare July 16, 2016 21:00
@floam floam changed the title Make fish build quieter Make fish Makefile build quieter Jul 16, 2016
@floam floam force-pushed the shutupmake branch 2 times, most recently from 1d4a064 to abbee7f Compare July 17, 2016 01:20
echo Compiling fish with CXXFLAGS=$(CXXFLAGS)
echo ===
Copy link
Contributor

Choose a reason for hiding this comment

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

The "===" is inconsistent with similar echo sequences. Personally I prefer surrounding meta comments with "====" (though I typically use ten or twenty equal-signs) so I have no objection to your including such a change but we should be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, I thought I put === on every header. I updated it as you suggested. .

$(INSTALL) -m 644 $$i $(DESTDIR)$(mandir)/man1/; \
true; \
done;
$(foreach i, $(MANUALS), $(INSTALL) -m 644 $(i) $(DESTDIR)$(mandir)/man1/;)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this change is the reason Travis is failing. Perhaps revert this portion.

Copy link
Member Author

@floam floam Jul 17, 2016

Choose a reason for hiding this comment

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

It was a combination of this and something else. It's working now.

@krader1961
Copy link
Contributor

I love the idea and don't have any objections once you fix the Travis failures and the minor style inconsistency I noted. I would love to be able to "make fish" and not have to wade through all the individual commands in order to see the warnings and errors. I wouldn't mind seeing the -Wignored-qualifiers addition be made permanent.

@zanchey
Copy link
Member

zanchey commented Jul 17, 2016

This sounds like a good idea, and I tried to do something similar a few years ago.

There's already a quasi-standard for this sort of approach - Automake silent rules & its pure-Make implementation in Linux's Kbuild.

In particular, I think it would be helpful if make V=1 or similar turned the full printing of rules back on - I use this quite a lot to get the full commandline and then make small modifications such as using -E.

@floam
Copy link
Member Author

floam commented Jul 17, 2016

I see now that probably I should also mimic the same output that you see at that linked article and that PCRE2 is using - I hadn't tried a build that would cause that to be built at the same time until just now but these should flow together:

  CXX      obj/file.o
clang++ [src/util.cpp]
clang++ [src/wcstringutil.cpp]
clang++ [src/wgetopt.cpp]
clang++ [src/wildcard.cpp]
clang++ [src/wutil.cpp]
  CC       src/libpcre2_32_la-pcre2_auto_possess.lo
  CC       src/libpcre2_32_la-pcre2_compile.lo
  CC       src/libpcre2_32_la-pcre2_config.lo
  CC       src/libpcre2_32_la-pcre2_context.lo
  CC       src/libpcre2_32_la-pcre2_dfa_match.lo

@krader1961
Copy link
Contributor

@zanchey I'm not thrilled about the solution documented in the link you provided. I'd prefer that a silent build be the default without having to provide a flag to the build tools. I do recognize that sometimes you need to see the actual commands being run. Such as when I implemented the make lint target. But I think it's sufficient to be able to just edit the Makefile and change the one line that makes the build silent. That, however, should be added to the CONTRIBUTING.md file.

@zanchey
Copy link
Member

zanchey commented Jul 17, 2016

We can make the silent build be the default, that's what the Linux Kbuild stuff does.

@floam
Copy link
Member Author

floam commented Jul 18, 2016

Here's what the PR is currently displaying like (though I noticed a couple things that look wrong that are changed in the next commit.)

I got rid of the baloney "now compiling x" headers - most of the objects are shared and the final linking for different binaries can just have a "CXXLD" line and it's fine.

screenshot 2016-07-18 at 9 27 27 am

edit: updated image

@floam floam force-pushed the shutupmake branch 15 times, most recently from 70e2ffd to 7923c69 Compare July 18, 2016 14:07
@floam floam force-pushed the shutupmake branch 15 times, most recently from 4baaf9f to 079a3a7 Compare July 20, 2016 02:56
@floam
Copy link
Member Author

floam commented Jul 31, 2016

Do we want to merge this? Or try another approach?

Aaron Gyes added 2 commits July 31, 2016 12:24
There was a lot of very noisy output for things
we do not care about, particularly the echoing of clang commands,
installs, and doxygen output.

We now show output like " CXX     src/fish.o" and not much else
unless there is a problem.

Add mechanism to show e.g. CXXFLAGS variables at top of build.

Improve make docs output

Highlight FISH_BUILD_VERSION

FISH_BUILD_VERSION is yellow.
Run ./configure with -q
Instead of using @ directly most of the time, use $(v) which can be
'' or @ controlled by V. Defaults to 0. make V=1 for a verbose make.
floam referenced this pull request Jul 31, 2016
@floam floam added this to the next-2.x milestone Aug 1, 2016
@floam floam self-assigned this Aug 2, 2016
@floam floam changed the title Make fish Makefile build quieter Make fish build more quietly. Aug 2, 2016
@floam floam merged commit 01f09cf into fish-shell:master Aug 2, 2016
@floam floam added the release notes Something that is or should be mentioned in the release notes label Aug 3, 2016
@krader1961 krader1961 modified the milestones: fish 2.4.0, next-2.x Sep 3, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement packaging release notes Something that is or should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants