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

Add an (Open Source) license? #2

Open
veprbl opened this issue Jul 16, 2022 · 2 comments · Fixed by #183 · May be fixed by #1321
Open

Add an (Open Source) license? #2

veprbl opened this issue Jul 16, 2022 · 2 comments · Fixed by #183 · May be fixed by #1321
Labels
topic: documentation Improvements or additions to documentation topic: reconstruction

Comments

@veprbl
Copy link
Member

veprbl commented Jul 16, 2022

The project lacks COPYING/LICENSE file.

@faustus123
Copy link
Contributor

This is an issue that has not been discussed and decided on at a collaboration level. (i.e. the specific license we should use) Some of the software here will be largely copied from the Juggler base, but then modified to fit the new framework. That may require copying the license used for the original code. I'm happy to do whatever, but it needs to be endorsed by the community. I agree that we should probably resolve this within the 1-2 weeks.

@wdconinc
Copy link
Contributor

I want to point out that https://github.com/eic/EICrecon/blob/main/RECON/plugins/BarrelEMCal/JFactory_EcalBarrelRawCalorimeterHit.cc is clearly inspired by LGPL code (including copied code segments) and that you are getting close to violation territory, if you are not already in it.

With Juggler we went through this decision process and decided on LGPL. I would ask you to respect this decision and follow the requirements of the LGPL if you will be copying code and making it available in this repository. That does indeed mean licensing (at least those parts) under LGPL.

@wdconinc wdconinc mentioned this issue Oct 5, 2022
14 tasks
@veprbl veprbl linked a pull request Oct 5, 2022 that will close this issue
14 tasks
veprbl added a commit that referenced this issue Jul 5, 2023
…tead of std::vector

This fixes memory leaks like:

```
Direct leak of 407056 byte(s) in 50882 object(s) allocated from:
    #0 0x55e48bee007d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 5069f7d2185cef71541ac1e93e2f6643618de2c4)
    #1 0x7fa316881fc5 in CalorimeterHitDigi::signal_sum_digi() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:254:23
    #2 0x7fa3168807eb in CalorimeterHitDigi::AlgorithmProcess() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:127:9
    #3 0x7fa3150b2e0c in RawCalorimeterHit_factory_LFHCALRawHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/FHCAL/RawCalorimeterHit_factory_LFHCALRawHits.h:86:9
    #4 0x7fa315a0fcc1 in eicrecon::JFactoryPodioT<edm4hep::RawCalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13
    #5 0x7fa315a4e8a8 in JFactoryT<edm4hep::RawCalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13
    #6 0x7fa315a4d87b in std::vector<edm4hep::RawCalorimeterHit const*, std::allocator<edm4hep::RawCalorimeterHit const*> > JEvent::Get<edm4hep::RawCalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27
```
veprbl added a commit that referenced this issue Jul 5, 2023
…tead of std::vector

This fixes memory leaks like:

    Direct leak of 389136 byte(s) in 48642 object(s) allocated from:
        #0 0x562ee9bce07d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 32bcf2da69e5409b22003a41cdb3023b1ebdbb21)
        #1 0x7fe7e2189715 in CalorimeterHitReco::AlgorithmProcess() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitReco.cc:240:20
        #2 0x7fe7e114a70c in CalorimeterHit_factory_EcalBarrelScFiRecHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelScFiRecHits.h:88:9
        #3 0x7fe7e133f8a1 in eicrecon::JFactoryPodioT<edm4eic::CalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13
        #4 0x7fe7e1352148 in JFactoryT<edm4eic::CalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13
        #5 0x7fe7e135096b in std::vector<edm4eic::CalorimeterHit const*, std::allocator<edm4eic::CalorimeterHit const*> > JEvent::Get<edm4eic::CalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27
veprbl added a commit that referenced this issue Jul 5, 2023
…ad of std::vector

This fixes a memory leak:

    Direct leak of 640736 byte(s) in 80092 object(s) allocated from:
        #0 0x562ee9bce07d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 32bcf2da69e5409b22003a41cdb3023b1ebdbb21)
        #1 0x7fe7e11690b2 in ImagingPixelReco::execute() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/ImagingPixelReco.h:133:32
        #2 0x7fe7e11674e9 in CalorimeterHit_factory_EcalBarrelImagingRecHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelImagingRecHits.h:65:9
        #3 0x7fe7e133f8a1 in eicrecon::JFactoryPodioT<edm4eic::CalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13
        #4 0x7fe7e1352148 in JFactoryT<edm4eic::CalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13
        #5 0x7fe7e135096b in std::vector<edm4eic::CalorimeterHit const*, std::allocator<edm4eic::CalorimeterHit const*> > JEvent::Get<edm4eic::CalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27
veprbl added a commit that referenced this issue Jul 23, 2023
…#741)

This fixes memory leaks like:

```
Direct leak of 407056 byte(s) in 50882 object(s) allocated from:
    #0 0x55e48bee007d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 5069f7d2185cef71541ac1e93e2f6643618de2c4)
    #1 0x7fa316881fc5 in CalorimeterHitDigi::signal_sum_digi() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:254:23
    #2 0x7fa3168807eb in CalorimeterHitDigi::AlgorithmProcess() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:127:9
    #3 0x7fa3150b2e0c in RawCalorimeterHit_factory_LFHCALRawHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/FHCAL/RawCalorimeterHit_factory_LFHCALRawHits.h:86:9
    #4 0x7fa315a0fcc1 in eicrecon::JFactoryPodioT<edm4hep::RawCalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13
    #5 0x7fa315a4e8a8 in JFactoryT<edm4hep::RawCalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13
    #6 0x7fa315a4d87b in std::vector<edm4hep::RawCalorimeterHit const*, std::allocator<edm4hep::RawCalorimeterHit const*> > JEvent::Get<edm4hep::RawCalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27
```

Leaks were discovered by LSAN that was enabled on CI recently #514.

Alternative way is to free the memory manually #731, or fix #742.
wdconinc added a commit that referenced this issue Jul 31, 2023
### Briefly, what does this PR introduce?
This clarifies that the algorithms (and arguably everything else here)
was always LGPL as per the upstream license conditions of juggler.

### What kind of change does this PR introduce?
- [x] Bug fix (issue #2, in part)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.
@wdconinc wdconinc reopened this Jul 31, 2023
github-merge-queue bot pushed a commit that referenced this issue Sep 16, 2023
### Briefly, what does this PR introduce?
Fixes overlow when running `eicrecon -c`, `--configs` (display
configuration parameters). For small `max_key_length`,
`max_key_length-4` wraps around.

```
Configuration Parameters:
terminate called after throwing an instance of 'std::length_error'
  what():  basic_string::_M_create

Program received signal SIGABRT, Aborted.
0x00007ffff7aacd3c in ?? () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) bt
#0  0x00007ffff7aacd3c in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007ffff7a5df32 in raise () from /lib/x86_64-linux-gnu/libc.so.6
#2  0x00007ffff7a48472 in abort () from /lib/x86_64-linux-gnu/libc.so.6
#3  0x00007ffff7cc2919 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007ffff7ccde1a in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007ffff7ccde85 in std::terminate() () from /lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00007ffff7cce0d8 in __cxa_throw () from /lib/x86_64-linux-gnu/libstdc++.so.6
#7  0x00007ffff7cc51e9 in std::__throw_length_error(char const*) () from /lib/x86_64-linux-gnu/libstdc++.so.6
#8  0x00007ffff7d648b9 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long) () from /lib/x86_64-linux-gnu/libstdc++.so.6
#9  0x00007ffff7d6495c in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct(unsigned long, char) () from /lib/x86_64-linux-gnu/libstdc++.so.6
#10 0x0000555555564476 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string<std::allocator<char> > (this=0x7fffffff6610, __n=18446744073709551615, __c=32 ' ', __a=...)
    at /usr/include/c++/12/bits/basic_string.h:659
#11 0x0000555555560b3e in jana::PrintConfigParameters (app=0x55555560e000) at /home/wdconinc/git/EICrecon/.worktree/ci-mt/src/utilities/eicrecon/eicrecon_cli.cpp:328
#12 0x00005555555611fa in jana::Execute (app=0x55555560e000, options=...) at /home/wdconinc/git/EICrecon/.worktree/ci-mt/src/utilities/eicrecon/eicrecon_cli.cpp:362
#13 0x000055555555b94f in main (narg=4, argv=0x7fffffff6ac8) at /home/wdconinc/git/EICrecon/.worktree/ci-mt/src/utilities/eicrecon/eicrecon.cc:60
```

### What kind of change does this PR introduce?
- [x] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.

---------

Co-authored-by: Dmitry Kalinkin <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Sep 17, 2023
### Briefly, what does this PR introduce?
A memory leak was caused by creating a new pointer `cl`, then push_back
`*cl`, but not deleting the pointer. This modifies the algorithm to use
objects instead of pointers. RVO and podio object encapsulation should
ensure performance is not affected.

Leak sanitizer output resolved:
```
Direct leak of 108512 byte(s) in 13564 object(s) allocated from:
    #0 0x55cbc7e9fded in operator new(unsigned long) (/home/wdconinc/git/EICrecon/.worktree/main/bin/eicrecon+0xeaded) (BuildId: 4702f11c9afe7b8bff422367fb6901ebdee7ea40)
    #1 0x7f92f7afe251 in eicrecon::CalorimeterClusterRecoCoG::reconstruct(edm4eic::ProtoCluster const&) const /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:293:10
    #2 0x7f92f7af9dc9 in eicrecon::CalorimeterClusterRecoCoG::process(edm4eic::ProtoClusterCollection const*, edm4hep::SimCalorimeterHitCollection const*) /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:55:18
    #3 0x7f92f7cb9713 in eicrecon::CalorimeterClusterRecoCoG_factoryT::Process(std::shared_ptr<JEvent const> const&) /home/wdconinc/git/EICrecon/.worktree/main/src/factories/calorimetry/CalorimeterClusterRecoCoG_factoryT.h:73:48
    #4 0x7f92f7cc0e54 in JMultifactoryHelperPodio<edm4eic::MCRecoClusterParticleAssociation>::Process(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.1-txdpt66ozlvvdquuovpy44emsmsnuip4/include/JANA/JMultifactory.h:228:20
    #5 0x7f92ffc7ccf7 in JFactory::Create(std::shared_ptr<JEvent const> const&) (/usr/._local/jhttvx4bcm3wdo7jmamfg6pazkythsai/lib/libJANA.so+0x59cf7) (BuildId: e11aaf0447ef572861e6577e3cfce0750b7aa2c3)

Direct leak of 45664 byte(s) in 5708 object(s) allocated from:
    #0 0x55cbc7e9fded in operator new(unsigned long) (/home/wdconinc/git/EICrecon/.worktree/main/bin/eicrecon+0xeaded) (BuildId: 4702f11c9afe7b8bff422367fb6901ebdee7ea40)
    #1 0x7f92f7afe251 in eicrecon::CalorimeterClusterRecoCoG::reconstruct(edm4eic::ProtoCluster const&) const /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:293:10
    #2 0x7f92f7af9dc9 in eicrecon::CalorimeterClusterRecoCoG::process(edm4eic::ProtoClusterCollection const*, edm4hep::SimCalorimeterHitCollection const*) /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:55:18
    #3 0x7f92f7cb9713 in eicrecon::CalorimeterClusterRecoCoG_factoryT::Process(std::shared_ptr<JEvent const> const&) /home/wdconinc/git/EICrecon/.worktree/main/src/factories/calorimetry/CalorimeterClusterRecoCoG_factoryT.h:73:48
    #4 0x7f92f7cbac34 in JMultifactoryHelperPodio<edm4eic::Cluster>::Process(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.1-txdpt66ozlvvdquuovpy44emsmsnuip4/include/JANA/JMultifactory.h:228:20
    #5 0x7f92ffc7ccf7 in JFactory::Create(std::shared_ptr<JEvent const> const&) (/usr/._local/jhttvx4bcm3wdo7jmamfg6pazkythsai/lib/libJANA.so+0x59cf7) (BuildId: e11aaf0447ef572861e6577e3cfce0750b7aa2c3)
```

### What kind of change does this PR introduce?
- [x] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.

Co-authored-by: Dmitry Kalinkin <[email protected]>
github-merge-queue bot pushed a commit that referenced this issue Sep 17, 2023
### Briefly, what does this PR introduce?
When there are no track tips, we should delete the
ActsExamples::Trajectories before moving on to the next one.

Fixes:
```
Direct leak of 128928 byte(s) in 1343 object(s) allocated from:
    #0 0x556fe8256ded in operator new(unsigned long) (/home/wdconinc/git/EICrecon/.worktree/main/bin/eicrecon+0xeaded) (BuildId: 7089f1e0b4fe65f89c79d38a99df5a0d1554ba34)
    #1 0x7f0f0594cb83 in eicrecon::CKFTracking::process(boost::container::flat_multiset<std::reference_wrapper<ActsExamples::IndexSourceLink const>, ActsExamples::detail::CompareGeometryId, void> const&, std::vector<std::variant<Acts::Measurement<Acts::BoundIndices, 1ul>, Acts::Measurement<Acts::BoundIndices, 2ul>, Acts::Measurement<Acts::BoundIndices, 3ul>, Acts::Measurement<Acts::BoundIndices, 4ul>, Acts::Measurement<Acts::BoundIndices, 5ul>, Acts::Measurement<Acts::BoundIndices, 6ul> >, std::allocator<std::variant<Acts::Measurement<Acts::BoundIndices, 1ul>, Acts::Measurement<Acts::BoundIndices, 2ul>, Acts::Measurement<Acts::BoundIndices, 3ul>, Acts::Measurement<Acts::BoundIndices, 4ul>, Acts::Measurement<Acts::BoundIndices, 5ul>, Acts::Measurement<Acts::BoundIndices, 6ul> > > > const&, edm4eic::TrackParametersCollection const&) /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/tracking/CKFTracking.cc:170:41
    #2 0x7f0f0514f9c3 in eicrecon::CKFTracking_factory::Process(std::shared_ptr<JEvent const> const&) /home/wdconinc/git/EICrecon/.worktree/main/src/global/tracking/CKFTracking_factory.cc:74:84
    #3 0x7f0f0524d104 in JMultifactoryHelperPodio<edm4eic::TrackParameters>::Process(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.1-txdpt66ozlvvdquuovpy44emsmsnuip4/include/JANA/JMultifactory.h:228:20
    #4 0x7f0f0cec0cf7 in JFactory::Create(std::shared_ptr<JEvent const> const&) (/usr/._local/jhttvx4bcm3wdo7jmamfg6pazkythsai/lib/libJANA.so+0x59cf7) (BuildId: e11aaf0447ef572861e6577e3cfce0750b7aa2c3)
```

### What kind of change does this PR introduce?
- [x] Bug fix (issue: memory leak)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.

Co-authored-by: Dmitry Kalinkin <[email protected]>
@ruse-traveler ruse-traveler added topic: documentation Improvements or additions to documentation topic: reconstruction labels Nov 1, 2023
@wdconinc wdconinc linked a pull request Mar 5, 2024 that will close this issue
7 tasks
@wdconinc wdconinc linked a pull request Mar 5, 2024 that will close this issue
7 tasks
ssedd1123 added a commit that referenced this issue Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: documentation Improvements or additions to documentation topic: reconstruction
Projects
4 participants