search

star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

Remove StRoot/StShadowMaker and "shadow" option #153

Closed plexoos closed 2 years ago

plexoos commented 2 years ago

This code cannot be used so I propose to remove it. Alternatively, the authors could fix the implementation by providing the valid code in StRoot/StShadowMaker but my understanding from #136 is that they are fine with just removing it.

plexoos commented 2 years ago

As you wish. You can remove the corresponding entry from the CODEOWNERS file in order to avoid automatic notifications https://github.com/star-bnl/star-sw/blob/68ae73a95f62da13742401879f26f5646727954b/.github/CODEOWNERS#L84

klendathu2k commented 2 years ago

That is not quite my reading of the discussion in #136.

StShadowMaker was used in the blinding/unblinding of data for the isobar data. That is the reason that the implementation file is encrypted. This code is (I believe) needed in order to be able to read / process the isobar data, and thus we need to maintain this in our library releases. Now that the data has been unblinded, it is not clear to me that StShadowMaker should remain encrypted. But that decision should be taken up as a topic for an S&C meeting.

plexoos commented 2 years ago

Just like I said:

Alternatively, the authors could fix the implementation by providing the valid code in StRoot/StShadowMaker

Who is the author? Jerome? @jlauret

jlauret commented 2 years ago

This code does not need to be deployed (it is not part of the chain except the blind analysis). We can leave it encrypted and never use in a checkout. Only the header can / should be deployed. Furthermore, I no longer have responsibility for the fate of any code in STAR remember? So, while the suggestion is already to leave it "as-is" and not checkout upon new library release (only the header file is needed, not the code itself), @genevb should IMO be the new owner of this piece of code.

genevb commented 2 years ago

A couple things that appear to need clarifying:

EDIT: I see Jerome responded at about the same time I did, so there is some overlap in what we have said.

plexoos commented 2 years ago

We can leave it encrypted and never use in a checkout.

Why? It cannot be compiled, it cannot be used. Why do you want to complicate the life of our users by requiring a sub-standard clone procedure? I hope you do realize that the production team is not the only user who can clone and compile the entire repo?

klendathu2k commented 2 years ago

Alternative to removing it: modify cons to ignore by default.

On 2021-09-22 10:21, Dmitri Smirnov wrote:

We can leave it encrypted and never use in a checkout.

Why? It cannot be compiled, it cannot be used. Why do you want to complicate the life of our users by requiring a sub-standard clone procedure? I hope you do realize that the production team is not the only user who can clone and compile the entire repo?

-- You are receiving this because your review was requested. Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. Triage notifications on the go with GitHub Mobile for iOS [3] or Android [4].

Links:

[1] https://github.com/star-bnl/star-sw/pull/153#issuecomment-924979389 [2] https://github.com/notifications/unsubscribe-auth/ANL4LVAAZVEZ7ECDJ75E6FTUDHQ7ZANCNFSM5ERJ4ESA [3] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!RzWee7JdwRXy1egsmk27CY-24ccj2KCv1EHf7OtofvHiQXzE9gZYPPy6y4-Y_Vk$ [4] https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!RzWee7JdwRXy1egsmk27CY-24ccj2KCv1EHf7OtofvHiQXzE9gZYPPy6av4CB70$

jlauret commented 2 years ago

Alternative to removing it: modify cons to ignore by default.

Or rename .cxx.enc and move on. So many possibilities ...

plexoos commented 2 years ago

modify cons to ignore by default.

Exactly my proposal #69 since July 26

genevb commented 2 years ago

As you wish. You can remove the corresponding entry from the CODEOWNERS file in order to avoid automatic notifications

https://github.com/star-bnl/star-sw/blob/68ae73a95f62da13742401879f26f5646727954b/.github/CODEOWNERS#L84

My understanding != my wish And apparently CODEOWNERS != responsibility for maintaining code (this point was made in #69 )

plexoos commented 2 years ago

My understanding != my wish

It's up to you

And apparently CODEOWNERS != responsibility for maintaining code (this point was made in #69 )

Your point != policy adopted by the collaboration. If there is a document describing the procedure of who and how can modify the code then please let us know.