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.

Sign up for GitHub

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

Jump to bottom

[tmva] When using DNN_USE_CBLAS, CMakeLists should link publicly to gsl instead of privately #14263

Closed
1 task done
ferdymercury opened this issue Dec 18, 2023 · 1 comment · Fixed by #14330
Closed
1 task done

[tmva] When using DNN_USE_CBLAS, CMakeLists should link publicly to gsl instead of privately #14263

ferdymercury opened this issue Dec 18, 2023 · 1 comment · Fixed by #14330
Assignees
@lmoneta
Labels
bug in:TMVA
Projects
Fixed in 6.32.00

Comments

@ferdymercury
Copy link
Collaborator

Check duplicate issues.

  • Checked for duplicates

Description

Bug found by Attilah here: root-project/root-docker#64 (comment)

The only place where gsl is included publicly in ROOT is in TMVA:

grep -r gsl/ /opt/root/*
`include/TMVA/DNN/Architectures/Cpu/Blas.h:#include "gsl/gsl_cblas.h"`

So cmake should link publicly to that library to make it a dependency, rather than privately.

Reproducer

root-project/root-docker#64

ROOT version

ROOT v6.30/02
Built for linuxx8664gcc on Nov 27 2023, 19:50:38
From tags/v6-30-02@
With c++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
Binary directory: /opt/root/bin

Installation method

docker

Operating system

Ubuntu 22.04

Additional context

No response
guitargeek added a commit to guitargeek/root that referenced this issue Jan 11, 2024
@guitargeek
[TMVA] Move tmva/tmva/inc/TMVA/DNN/Architectures/Cpu/Blas.h to src
2ef68d7 
The `Blas.h` header is only used by the tmva-cpu implementation, so it
should be made private by moving it to `src`.

The problem with having it in `inc` was that this header included GSL,
and therefore GSL appeared to be a public dependency of TMVA, which is
not true.

Closes root-project#14263.
@guitargeek guitargeek mentioned this issue Jan 11, 2024
Merged
@guitargeek
Copy link
Contributor

Thanks for reporting! That Blas.h should not have been public to begin with. I opened a PR where I move if to src, please review if you have time.

guitargeek added a commit that referenced this issue Jan 11, 2024
@guitargeek
[TMVA] Move tmva/tmva/inc/TMVA/DNN/Architectures/Cpu/Blas.h to src
7219853 
The `Blas.h` header is only used by the tmva-cpu implementation, so it
should be made private by moving it to `src`.

The problem with having it in `inc` was that this header included GSL,
and therefore GSL appeared to be a public dependency of TMVA, which is
not true.

Closes #14263.
@guitargeek guitargeek added this to Issues in Fixed in 6.32.00 via automation Jan 11, 2024
maksgraczyk pushed a commit to maksgraczyk/root that referenced this issue Jan 12, 2024
@guitargeek @maksgraczyk
[TMVA] Move tmva/tmva/inc/TMVA/DNN/Architectures/Cpu/Blas.h to src
5f30e4b 
The `Blas.h` header is only used by the tmva-cpu implementation, so it
should be made private by moving it to `src`.

The problem with having it in `inc` was that this header included GSL,
and therefore GSL appeared to be a public dependency of TMVA, which is
not true.

Closes root-project#14263.
vgvassilev pushed a commit to vgvassilev/root that referenced this issue Feb 8, 2024
@guitargeek @vgvassilev
[TMVA] Move tmva/tmva/inc/TMVA/DNN/Architectures/Cpu/Blas.h to src
ab0db0d 
The `Blas.h` header is only used by the tmva-cpu implementation, so it
should be made private by moving it to `src`.

The problem with having it in `inc` was that this header included GSL,
and therefore GSL appeared to be a public dependency of TMVA, which is
not true.

Closes root-project#14263.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lmoneta lmoneta

Labels
bug in:TMVA
Projects
No open projects
Fixed in 6.32.00
Issues
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[TMVA] Move tmva/tmva/inc/TMVA/DNN/Architectures/Cpu/Blas.h to src guitargeek/root
3 participants
@lmoneta @guitargeek @ferdymercury