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

[RF] Disable RooFit banner with CMAKE_CXX_FLAGS option __ROOFIT_NOBANNER by default #9954

Closed
matthewfeickert opened this issue Feb 22, 2022 · 10 comments · Fixed by #9965
Closed

Comments

@matthewfeickert
Copy link
Collaborator

matthewfeickert commented Feb 22, 2022

Explain what you would like to see improved

When RooFit code is run it will print a copyright "banner"

/// Print RooFit banner.
void doBanner() {
#ifndef __ROOFIT_NOBANNER
if (gEnv->GetValue("RooFit.Banner", 1) == 0)
return;
/// RooFit version tag.
constexpr char VTAG[] = "3.60";
std::cout << '\n'
<< "\033[1mRooFit v" << VTAG << " -- Developed by Wouter Verkerke and David Kirkby\033[0m " << '\n'
<< " Copyright (C) 2000-2013 NIKHEF, University of California & Stanford University" << '\n'
<< " All rights reserved, please read http://roofit.sourceforge.net/license.txt" << '\n'
<< std::endl;
#endif
}

$ python -c 'from ROOT import RooFit'  # Get banner

RooFit v3.60 -- Developed by Wouter Verkerke and David Kirkby
                Copyright (C) 2000-2013 NIKHEF, University of California & Stanford University
                All rights reserved, please read http://roofit.sourceforge.net/license.txt

when RooFit was being developed in the early 2000s this might have been about the only way to get open source software recognized and (hopefully!) cited (I was not involved with scientific open source then, so someone who was should feel free to correct me if I have the history wrong). However, in the time since there has been significant improvements in the citation and scholarship of open source software (e.g., some key points being the existence of JOSS and CITATION.cff native support on GitHub). As RooFit is also effectively absorbed into ROOT (I hope this is safe to say given the development history on the rootfit module in ROOT and that the RooFit development GitHub org does their development in a fork of ROOT) and any development as a solo project on SourceForge has long halted, the banner is a recurring pain point and nuisance for many users.

The banner can be disabled at runtime through adding RooFit.Banner: no to .rootrc

$ python -c 'from ROOT import RooFit'  # Get banner

RooFit v3.60 -- Developed by Wouter Verkerke and David Kirkby
                Copyright (C) 2000-2013 NIKHEF, University of California & Stanford University
                All rights reserved, please read http://roofit.sourceforge.net/license.txt

$ echo 'RooFit.Banner: no' > ~/.rootrc
$ python -c 'from ROOT import RooFit'  # No banner now

or permanently disabled by passing the compile flag -D__ROOFIT_NOBANNER to RooFit at compile time and if building ROOT with CMake you can pass this flag to RooFit through the CMAKE_CXX_FLAGS option.

Disabling the RooFit banner with

-DCMAKE_CXX_FLAGS=-D__ROOFIT_NOBANNER

as a default and finding a more constructive and useful form of attribution to the RooFit developers seems like a much better solution then continuing to keep it around as a default. (Altering the .rootrc seems like a nice way to deal with legacy versions of ROOT, but is not a good reason to keep the banner.)

Optional: share how it could be improved

Make the CMake build option

-DCMAKE_CXX_FLAGS=-D__ROOFIT_NOBANNER

the default to disable the RooFit banner from being enabled by default.

In addition to disabling the banner, there should be some additional action to preserve the copyright and attribution to Wouter and David in a more useful manner, as well as update it to reflect the huge amount of work that has happened since 2013 (here I would suggest a CITATION.cff file. I also think ROOT should get one too, but that's another Issue.).

As Wouter doesn't appear to have a GitHub account (edit: I'm wrong, he's @wverkerke), I'll tag @egpbos as a public member of the RooFit development team (apologies if I should have tagged someone else) as well as @dkirkby to see if they have any particular thoughts on the matter.

To Reproduce

To trigger the RooFit banner simply run any piece of code that uses any part of RooFit. Example:

$ python -c 'from ROOT import RooFit'  # Get banner

RooFit v3.60 -- Developed by Wouter Verkerke and David Kirkby
                Copyright (C) 2000-2013 NIKHEF, University of California & Stanford University
                All rights reserved, please read http://roofit.sourceforge.net/license.txt

Setup

For the examples I've shown above are using ROOT v6.24/06 built from source

$ root --version
ROOT Version: 6.24/06
Built for linuxx8664gcc on Feb 18 2022, 00:26:00
From tags/v6-24-06@v6-24-06

Additional context

@dantrim
Copy link

dantrim commented Feb 22, 2022

I +1 the idea that the banner should be disabled by default.

@jpivarski
Copy link
Contributor

I generally expect libraries to be silent and applications to write log files. If an application uses a dozen libraries, users don't want to see a dozen banners scroll past their terminal whenever they run it.

A mark of a library's success is when it gets into the infrastructure in ways that downstream users might not even know about. If that's the goal here—for RooFit to be a library that other libraries or applications are built upon—then it shouldn't print its banner on startup.

Although RooFit's API was designed with data analysts in mind (i.e. as an application), in recent years it has de-facto become a library with the likes of HistFactory, Combine, and lots of mini-frameworks built on top of it. If it were me, I would take that as a good sign and embrace it by becoming more library-like.

(I have the same opinion of FastJet's banner, but that's another story.)

@dkirkby
Copy link
Contributor

dkirkby commented Feb 23, 2022

I am fine with suppressing the banner by default. Thanks for checking.
I will email Wouter to make sure he is aware of this thread.

@Axel-Naumann
Copy link
Member

Axel-Naumann commented Feb 23, 2022

Thanks, @dkirkby !

If it's disabled by default then I am positive nobody will go the extra mile to enable it in their builds. Which means we should remove the banner and the infrastructure to disable it, through rootrc or a CMake variable.

@matthewfeickert would you be able to propose a PR?

@lmoneta
Copy link
Member

lmoneta commented Feb 23, 2022

Thank you all for your comments. I will discuss also with Wouter and the other RooFit developers next week and we will then fix this.

@matthewfeickert
Copy link
Collaborator Author

If it's disabled by default then I am positive nobody will go the extra mile to enable it in their builds. Which means we should remove the banner and the infrastructure to disable it, through rootrc or a CMake variable.

I would agree.

would you be able to propose a PR?

I can outline the sections to be removed in this (or another) Issue for discussion and then make one if that's helpful @Axel-Naumann. Though I take it from @lmoneta's comment that they would prefer to wait. Is that correct?

@guitargeek
Copy link
Contributor

guitargeek commented Feb 23, 2022

Thanks for opening this issue. It was also on my personal agenda for the next ROOT release anyway :) Indeed I wanted to bring it up with Wouter and the others. No serious library nowadays has such a banner, why should RooFit have it.

@Axel-Naumann, @matthewfeickert, I will take care of opening a PR for this, as I already have the commit ready.

guitargeek added a commit to guitargeek/root that referenced this issue Feb 23, 2022
See GitHub issue root-project#9954 for nice post making a case for this.
@dkirkby
Copy link
Contributor

dkirkby commented Feb 23, 2022

No serious library nowadays has such a banner, why should RooFit have it.

I agree with that assessment today, but the open-source landscape was quite different when that banner originated, over 20 years ago.

@matthewfeickert
Copy link
Collaborator Author

I agree with that assessment today, but the open-source landscape was quite different when that banner originated, over 20 years ago.

Indeed. I really think that there should be a discussion (maybe in another Issue?) of making sure that we credit RooFit in a nicer more modern format. Its been a workhorse for the community for a long time and we should be citing it more!

guitargeek added a commit that referenced this issue Feb 26, 2022
See GitHub issue #9954 for nice post making a case for this.
@guitargeek guitargeek linked a pull request Apr 13, 2022 that will close this issue
@guitargeek
Copy link
Contributor

Fixed by #9965 in the 6.28 development cycle.

@guitargeek guitargeek added this to the 6.28/00 milestone Apr 13, 2022
@guitargeek guitargeek added this to Issues in Fixed in 6.28/00 via automation Apr 13, 2022
@guitargeek guitargeek removed this from the 6.28/00 milestone Apr 13, 2022
@guitargeek guitargeek changed the title Disable RooFit banner with CMAKE_CXX_FLAGS option __ROOFIT_NOBANNER by default [RF] Disable RooFit banner with CMAKE_CXX_FLAGS option __ROOFIT_NOBANNER by default Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants