star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

Updates for sl6 compile #129

Closed jml985 closed 2 years ago

jml985 commented 2 years ago

Nope, the problem is supposedly that the copy constructor is private in TH1 in root6. I don't know why. It turns out that I don't use the copy constructor anywhere though, so I leave it out... Oh. I suppose the asdf is a typo put in when I tested it. Should never come up, but still... should I redo the pull request?

-Jeff


From: Hongwei Ke @.> Sent: Monday, August 30, 2021 5:20 PM To: star-bnl/star-sw @.> Cc: Landgraf, Jeffery M. @.>; Author @.> Subject: Re: [star-bnl/star-sw] Updates for sl6 compile (#129)

@kehw commented on this pull request.


In OnlTools/Jevp/StJevpPlot/JevpPlot.hhttps://github.com/star-bnl/star-sw/pull/129#discussion_r698815308:

@@ -21,12 +21,15 @@ class PlotHisto : public TObject { PlotHisto(TH1 *hist = NULL); PlotHisto(PlotHisto &x);

+#ifdef PLOTHISTO_COPY_CONSTRUCTURE +asdf

Is this a typo?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/star-bnl/star-sw/pull/129#pullrequestreview-742080742, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKQGF6I4U7BDITG6VI6OARTT7PYYRANCNFSM5DCTFEQA. Triage notifications on the go with GitHub Mobile for iOShttps://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!SSbRJ-5q1odXP9CVxRk7QvUIuljf_U2mwqL-poabihCe4E3v4rUyxeAUSosW$ or Androidhttps://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!SSbRJ-5q1odXP9CVxRk7QvUIuljf_U2mwqL-poabihCe4E3v4rUyxQN0vu9v$.

plexoos commented 2 years ago

the problem is supposedly that the copy constructor is private in TH1 in root6.

Are you using ROOT6 in SL6?

jml985 commented 2 years ago

I don't use root6, but the reason this is being looked at is this general effort to get root6 working. (The code needs to compile under root6 even though it doesn't run in root6 I guess). There was also somebody else that put in a patch for this using the clone() function to properly implement the copy constructor. That is probably a better solution, though it doesn't matter because the copy constructor isn't used. But frankly this is a mess right now. I have no idea what code is in what repository, and in what status of "approval" any of it is in.

plexoos commented 2 years ago

There was also somebody else that put in a patch for this using the clone() function to properly implement the copy constructor. That is probably a better solution, though it doesn't matter because the copy constructor isn't used.

If you are sure that the code is not used then a better solution would be to remove it completely. Even better if you can come up with a test that can confirm this so we can add it to our CI.

Without such test we'll just take your word for it and update #101 by removing the unused code

But frankly this is a mess right now.

Do you have any constructive suggestions on how to avoid a mess? We would like to hear

I have no idea what code is in what repository,

https://github.com/star-bnl/star-sw is the official repository. Other users can create forks and submit their changes from them.

and in what status of "approval" any of it is in.

This can be seen on the Pull Request page at https://github.com/star-bnl/star-sw/pulls

101 has been open for over two weeks now. AFAIK, there were no comments from you on the proposed changes.

jml985 commented 2 years ago

Its a mess because something like 6 people are involved in a completely trivial update to my code. I'm working on it, but have not yet been able to pull requests onto the proper computer in a fashion I can use. The code runs on a (2 specific computers), from a specific directories. It is possible to test certain features independently from the production code, but not all, so for really valid tests I'm stuck to the location of the production code, and to bringing down the production server and I am not facile enough with git to do this in place (and be sure to get back to the original code if needed).

For this code, it needs to compile under every environment because the histogram building code from the detectors is designed to be tested by any user from SDCC or anywhere else, and these developers need the dependencies from the rest of OnlTools/Jevp, but other than that it is not used by offline in any way.

The best way, for me, to deal with the problems us for somebody to let me know what they are and how to replicate them (In this case give me some instructions to build under root6) and then let me fix them.

The scheme of having several pending minor changes makes me very confused as to the workflow I'm supposed to take. What it seems like I need to do is:

  1. take down the production server
  2. download several patches into the code tree for this server. (Thus far I am failing at this. I'm trying to get this to work with "gh" but have not succeeded surely due to my ignorance. I'm get messages like: "> gh pr checkout 129 GraphQL error: Could not resolve to a PullRequest with the number of 129.")
  3. test that it works
  4. give the comments that pull request is fine by me
  5. then what? go back to the main branch and run with old code? stay on the pull request branch until the pull request is actually put into the main branch? And what happens if another pull request comes, or if I need to make a change before the request is accepted?
plexoos commented 2 years ago

At this point, I think we should just remove OnlTools/ from this repository and let the online group to deal with it the way they want. Indeed, this repository is for offline processing of the data and OnlTools does not really fit in it.

I modified the avail file in CVSROOT accordingly, so you should be able to commit to OnlTools in CVS

$ diff -u0 avail /afs/rhic.bnl.gov/star/packages/repository/CVSROOT/avail
--- avail       2021-07-14 13:05:39.000000000 -0400
+++ /afs/rhic.bnl.gov/star/packages/repository/CVSROOT/avail    2021-08-31 14:46:02.000000002 -0400
@@ -143,0 +144,2 @@
+avail   |jml,tonko,akio                        |StRoot/RTS
+avail   |jml,tonko,akio                        |OnlTools
jml985 commented 2 years ago

I'm not sure that works either, as I said, we need this in the standard compile because its crucial that the detector groups can update and test from the StJevpBuilder directories, and they depend upon StRoot as well.

I'm going to keep trying to figure out my technical problems with downloading the patches, but probably the best solution at this point is to throw away this patch (#129). Then, either (tell me how to test under root6, and I'll make sure it compiles) or (fix for root6 under your own best ideas) and accept the patches.

Once this is done, I'll pull the repository and test (and make additional fixes if the root6 changes break anything).

kehw commented 2 years ago

@plexoos do we have a standard way to setup ROOT6 environment on RCF? Is something like "setup ROOT 6.16.00" good enough for test what we are trying to do here?

plexoos commented 2 years ago

@plexoos do we have a standard way to setup ROOT6 environment on RCF? Is something like "setup ROOT 6.16.00" good enough for test what we are trying to do here?

This is all I know about ROOT6 on the farm https://github.com/star-bnl/star-sw/discussions/68

I thought you wanted to ask Jerome to add some setup lines in the group scripts. I don't have access to any of this

jml985 commented 2 years ago

I really don't understand this at all. I made three test commits to the "jml985/star-sw" repository checking my setup of my credentials. These showed up as "blah", "blah2", and "yeah" in this discussion. Why does this show up in the star-bnl/star-sw pull request log?

jml985 commented 2 years ago

@plexoos , @kehw : For the root6 stuff, if it is easy get it to compile and don't worry about whether I like it or not to put it in the repository. We don't need the Jevp server running for the next month or two, so once it compiles under root6 I can check it. Of course, if the fixes for root6 are more complicated or touch on the underlying behavior of the server I'll need to be able to compile under that environment to check it out...

jml985 commented 2 years ago

Also, I finally figured out getting gh to get the pull requests down, and can switch between the pull request branch and the main one easily. What is the process if I want to merge two pull requests to test together? Do I create a new local branch and then merge them on my local branch?

kehw commented 2 years ago

Hi Jeff, I do not have experience of merging two pull requests. I think a new branch maybe a cleaner way. If there is only a single file you want, you can check that signal file like,

(I am on branch A) > git checkout branch_B -- path/to/a/file

this will overwrite your local copy of the file