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] Remove RooFit banner from reference files #840

Closed

Conversation

henryiii
Copy link

@henryiii henryiii commented Mar 1, 2022

Matching root-project/root#9999 - backport of #834.

@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link

Build failed on mac11/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/roottest-pullrequests-build
See console output.

Errors:

  • [2022-03-01T14:55:38.664Z] FAILED: builtins/xrootd/XROOTD-prefix/src/XROOTD-stamp/XROOTD-download /Users/sftnight/build/workspace/roottest-pullrequests-build/build/builtins/xrootd/XROOTD-prefix/src/XROOTD-stamp/XROOTD-download
  • [2022-03-01T14:55:38.664Z] CMake Error at XROOTD-stamp/XROOTD-download-Release.cmake:49 (message):

@phsft-bot
Copy link

Build failed on mac1015/python3.
Running on macitois19.dyndns.cern.ch:/Users/sftnight/build/workspace/roottest-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\roottest-pullrequests-build
See console output.

Failing tests:

@matthewfeickert
Copy link

@guitargeek, as this missed release 6.26/02 can this get reviewed again so that root-project/root#9999 can go it for the next patch release?

@henryiii
Copy link
Author

Poke me if anything need to be done, I was under the impression it was ready.

@guitargeek
Copy link
Contributor

Hi, thanks for the ping!

Sorry for coming back to this so late. I forgot to let you know that it was decided that the banner removal will not be done in the 6.26 branch. So unfortunately, I have to close this PR.

At least in the upcoming 6.26 release, the banner is gone for good! I hope this compromise is acceptable for you.

@matthewfeickert
Copy link

matthewfeickert commented Apr 13, 2022

I forgot to let you know that it was decided that the banner removal will not be done in the 6.26 branch. So unfortunately, I have to close this PR.

Thanks for this information. Out of curiosity, is the discussion for the decision to wait till release 6.28 public?

@guitargeek
Copy link
Contributor

Hi! No the discussion was not public. The main argument is that patch releases are only for bugfixes, not to introduce behavior change. By removing the RooFit banner, we might cause hickups in user workflows just like here in roottests if people compare their logs to reference files. That should not happen in a patch release

@matthewfeickert
Copy link

matthewfeickert commented Apr 14, 2022

Thanks for clarifying. This is what I assumed. I'm not going to argue as you have more relevant things to do and you've already helped a lot in just getting this in (thank you for that!), but testing frameworks are not users and users should be using APIs not banners to extract information. While I appreciate that ROOT might want to be better about following SemVer then it has been in the past, it is hard to see this as a breaking change (but maybe it is sadly).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants