cons: Skip uncompilable package StShadowMaker

[plexoos]

The user can still define environment variable SKIP_DIRS to overwrite the default list of skipped subdirectories. And it is possible to build a skipped by default package as, e.g.:

cons +StShadowMaker

[plexoos]

Unfortunately, those punished by the long command are not always the ones who supposed to fix the issues.

https://github.com/star-bnl/star-sw/blob/ec78afedef5aba6ad076bb3820c4b1f3802d7bea/.github/CODEOWNERS#L62

[veprbl]

@plexoos I'm using the StJetFinder right now. Not sure what the issue with it is, I suppose the FastJet is not installed on CI?

[veprbl]

Just tested:

csh -c "starver SL21b; cons +StRoot/StJetFinder +StRoot/StJetMaker +StRoot/StSpinPool"

Doesn't fail on the current "main"

[genevb]

Not sure what the issue with it is

Success or failure of compilation isn't necessarily the motivation for excluding from standard compilation. It may have been introduced without anyone committing to be the maintainer, with an agreement to exclude it, as has been the case with the Pools. Is there a committed maintainer now?

[plexoos]

75 should be merged first. It should take care of the build issue

[plexoos]

This PR has been updated considerably since its origination. As of now, all packages in star-sw can be compiled including the previously excluded ones. Those have been taken care of and compile in both ROOT5 and ROOT6 environments as confirmed by our CI jobs. The only remaining exception is the StRoot/StShadowMaker package which is expected to be excluded from the build.

Just to remind, the purpose of this PR is to let the users simply run the cons command without either setting the SKIP_DIRS environment variable if they don't want to or specifying the exclusion patterns % on the command line.

If any comments or suggestions, please let me know.

Cc @genevb @klendathu2k

[genevb]

Hi, Dmitri

This PR has been updated considerably since its origination. As of now, all packages in star-sw can be compiled including the previously excluded ones. Those have been taken care of and compile in both ROOT5 and ROOT6 environments as confirmed by our CI jobs. The only remaining exception is the StRoot/StShadowMaker package which is expected to be excluded from the build.

Just to remind, the purpose of this PR is to let the users simply run the cons command without either setting the SKIP_DIRS environment variable if they don't want to or specifying the exclusion patterns % on the command line.

My thoughts:

This implies a requirement that these previously skipped directories will from now on be compilable, which is a responsibility that wasn't taken before now. The core issue isn't about whether these codes could be fixed so that they compile, but about who has taken this responsibility.

-Gene

[plexoos]

Hi Gene,

The production team can still skip the compilation of these (or any other packages) by defining the SKIP_DIRS. In fact, no action needs to be taken on your side if you want to keep the status quo for the builds on the farm. With the CI in place we are in general protected from changes breaking the code on the main branch. In other words, whoever touches the code would be responsible for making sure it compiles, and the infrastructure team is available to help if needed. This obviously implies that no changes should be accepted and merged onto the main branch if the CI jobs fail.

[veprbl]

Since the blinded analyses have been published, I think its fine to just fix or delete the maker.

[plexoos]

Yes, isobars was a fun game but it's over now. Deleting or fixing the maker is definitely an option.

[genevb]

Hi, Dmitri

On Sep 2, 2021, at 2:56 PM, Dmitri Smirnov @.**@.>> wrote:

With the CI in place we are in general protected from changes breaking the code on the main branch. In other words, whoever touches the code would be responsible for making sure it compiles, and the infrastructure team is available to help if needed. This obviously implies that no changes should be accepted and merged onto the main branch if the CI jobs fail.

While I do appreciate what CI provides in terms of quicker feedback on compilation, it does not provide much more on this specific point of code acceptance than the nightly build provides: AutoBuild never let un-skipped codes make it to DEV if they didn't compile. And the S&C Team reverted CVS commits if they were problematic (I can point to examples if desired). So that was not an unsolved problem.

Do you believe that all the fixes that have been made to those previously skipped directories over the last few weeks were resolved by backing out commits that people made some time ago?

-Gene

[genevb]

Since the blinded analyses have been published, I think its fine to just fix or delete the maker.

I don't think the results have been published yet, but I'm fine with relegating the implementation to the archives now: #136

[plexoos]

While I do appreciate what CI provides in terms of quicker feedback on compilation, it does not provide much more on this specific point of code acceptance than the nightly build provides: AutoBuild never let un-skipped codes make it to DEV if they didn't compile. And the S&C Team reverted CVS commits if they were problematic (I can point to examples if desired). So that was not an unsolved problem.

One problem with the workflow you describe is that a single user committing a faulty change can block the access to the DEV libraries for others submitting (non-faulty) changes at the same time. With human involvement it can also take longer to revert the offending code, especially when it is a partial revert or the original author is not readily available. With an automatic build-and-test-before-merge approach this problem does not exist.

Do you believe that all the fixes that have been made to those previously skipped directories over the last few weeks were resolved by backing out commits that people made some time ago?

No, most of the fixes were actually related to ROOT6. For ROOT5, most of the originally skipped packages did not require any adjustments to their code. So, I don't really understand why, for example, StSpinPool, StFgtPool, and StFwdTrackMaker are skipped in AutoBuild.

[plexoos]

I don't think the results have been published yet, but I'm fine with relegating the implementation to the archives now: #136

Great! Then the change to mgr/Construct proposed here is not really needed

[genevb]

Hi, Dmitri

On Sep 3, 2021, at 12:51 AM, Dmitri Smirnov @.**@.>> wrote:

While I do appreciate what CI provides in terms of quicker feedback on compilation, it does not provide much more on this specific point of code acceptance than the nightly build provides: AutoBuild never let un-skipped codes make it to DEV if they didn't compile. And the S&C Team reverted CVS commits if they were problematic (I can point to examples if desired). So that was not an unsolved problem.

One problem with the workflow you describe is that a single user committing a faulty change can block the access to the DEV libraries for others submitting (non-faulty) changes at the same time. With human involvement it can also take longer to revert the offending code, especially when it is a partial revert or the original author is not readily available. With an automatic build-and-test-before-merge approach this problem does not exist.

CI will help us do this better, but as I said, we dealt with this when necessary. You're improving on the existing solution.

You brought up CI as if it will enforce code maintenance, but my point is that it does not: it helps in the procedure of administratively controlling commits, but it doesn't force responsibility for maintenance when code stops compiling for other reasons. Such as...

Do you believe that all the fixes that have been made to those previously skipped directories over the last few weeks were resolved by backing out commits that people made some time ago?

No, most of the fixes were actually related to ROOT6.

Indeed. So you agree that it wasn't some inappropriate commit by authors of code in those directories that caused them to fail to compile. CI didn't make those code authors fix anything. Who fixed the codes? Who needed to be notified of the fixes or approved them?

For ROOT5, most of the originally skipped packages did not require any adjustments to their code.

Did any of the codes need fixed for inappropriate commits?

So, I don't really understand why, for example, StSpinPool, StFgtPool, and StFwdTrackMaker are skipped in AutoBuild.

StFwdTrackMaker does not compile in 64-bit and nothing has been done about that for a while. Do you think the code author made an inappropriate commit?

If you're going to require codes in the Pool directories to compile, why have the Pool hierarchy at all? Just move the subdirectories out from under the Pools.

-Gene

[veprbl]

CI will help us do this better, but as I said, we dealt with this when necessary. You're improving on the existing solution.

You brought up CI as if it will enforce code maintenance, but my point is that it does not: it helps in the procedure of administratively controlling commits, but it doesn't force responsibility for maintenance when code stops compiling for other reasons. Such as...

Why are you so preoccupied with forcing responsibility?

No, most of the fixes were actually related to ROOT6.

Indeed. So you agree that it wasn't some inappropriate commit by authors of code in those directories that caused them to fail to compile. CI didn't make those code authors fix anything. Who fixed the codes? Who needed to be notified of the fixes or approved them?

You are basically making the case for why working in an environment that can be unpredictably changed by a who-knows-who in a who-nows-how ways at a who-knows-when time makes such problems into a mess. If we work in properly defined isolated environments, then the person who changes the definition (i.e. Dockerfile) will submit change with a proper motivation for their change. Normally they also fix the minor problems, and can invite people from "CODEOWNERS" to collaborate on the substantial changes needed.

StFwdTrackMaker does not compile in 64-bit and nothing has been done about that for a while. Do you think the code author made an inappropriate commit?

Was committed 5 months ago https://github.com/star-bnl/star-sw/commits/main/StRoot/StFwdTrackMaker, never modified and is currently building fine on RCF.

If you're going to require codes in the Pool directories to compile, why have the Pool hierarchy at all? Just move the subdirectories out from under the Pools.

That is an idea to consider.

[genevb]

I think this question makes clear that we will not come to an agreement on this topic. -Gene

On Sep 3, 2021, at 6:20 AM, Dmitry Kalinkin @.**@.>> wrote:

Why are you so preoccupied with forcing responsibility?

[plexoos]

StFwdTrackMaker does not compile in 64-bit and nothing has been done about that for a while.

Let's investigate. Can you create an issue with details?

[genevb]

StFwdTrackMaker does not compile in 64-bit and nothing has been done about that for a while.

Let's investigate. Can you create an issue with details?

Done: #139

[plexoos]

Any further comments on this proposed change? Can anyone see any harm from introducing the default for SKIP_DIRS? (While the collaboration is deciding whether to keep StShadowMaker as is... #153)