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

US_Archive utils class #137

Merged
merged 13 commits into from
Oct 8, 2024
Merged

US_Archive utils class #137

merged 13 commits into from
Oct 8, 2024

Conversation

samo38
Copy link
Collaborator

@samo38 samo38 commented Oct 4, 2024

No description provided.

@samo38 samo38 requested a review from ehb54 October 4, 2024 01:00
@ehb54
Copy link
Owner

ehb54 commented Oct 4, 2024

@samo38
The US_Archive class is failing CodeQL checks on MPI & GUI

@ehb54
Copy link
Owner

ehb54 commented Oct 4, 2024

@samo38
There may be other issues with the CodeQL workflow , but likely needs a dnf install libarchive-devel for the container in the codeql yaml
The other option is to build a new base container with the libarchive-devel preinstalled and push it to dockerhub.

@ehb54
Copy link
Owner

ehb54 commented Oct 6, 2024

@samo38
Fixed the mpi build, but not the gui build. Note the codeql creates multiple containers - one for each analysis, so the apt install will needed to be included for the gui also.
Don't need it for somo, since somo isn't using libarchive.

@ehb54
Copy link
Owner

ehb54 commented Oct 7, 2024

Don't need it for somo, since somo isn't using libarchive.

@ehb54
Copy link
Owner

ehb54 commented Oct 7, 2024

@samo38
As i said earlier:

Don't need it for somo, since somo isn't using libarchive.

I doesn't hurt a lot to have it, just takes a bit longer for the analysis to run. At some point we may push against the limit of container time and have to provide our own container cloud for the analysis, which I'm hoping to avoid.

@samo38
Copy link
Collaborator Author

samo38 commented Oct 7, 2024

Sure! I just add it just in case. Will remove it! somo has its own copy of us_tar in the develop directory. Though it is not called anywhere.

@samo38
Copy link
Collaborator Author

samo38 commented Oct 7, 2024

To use it in the analyze-mpi, I can call us_archive directly instead of editing us_tar or leave it to use us_tar if it's not necessary now.

@ehb54
Copy link
Owner

ehb54 commented Oct 7, 2024

@samo38

To use it in the analyze-mpi, I can call us_archive directly instead of editing us_tar or leave it to use us_tar if it's not necessary now.

I'm concerned about the volume of testing required... lots of clusters, analysis methods etc.
Let's leave it for now and get this version merged.
I think we are ready after the codeql analyses finish.

@samo38
Copy link
Collaborator Author

samo38 commented Oct 7, 2024

Places where US_Tar are called:

using system tar:

//Archive using US_Tar [does NOT work for filename lengths >=100 char]

@samo38
Copy link
Collaborator Author

samo38 commented Oct 7, 2024

So If you'd prefer to leave us_mpi_analysis to use US_Tar for now, do I delete installing libarchive from github workflow? Although it is quit fast to install it with apt-get.

@ehb54
Copy link
Owner

ehb54 commented Oct 7, 2024

So If you'd prefer to leave us_mpi_analysis to use US_Tar for now, do I delete installing libarchive from github workflow? Although it is quit fast to install it with apt-get.

I don't see any changes to us_mpi_analysis programs themselves, but the changes did require the codeql apt-get for mpi to succeed. So leave the apt-get for mpi.

I think we can migrate us_mpi_analysis to use us_archive, but want to keep it as a separate issue/pull request due to the required testing. This way, we can get your legacy converter in-place and take our time with the us_mpi_analysis.

@samo38
Copy link
Collaborator Author

samo38 commented Oct 7, 2024

Yes you're right! By the way us_mpi_analysis needs the utils to be complied first so libarchive is needed anyway.

@ehb54 ehb54 merged commit 2b6fc69 into ehb54:master Oct 8, 2024
5 checks passed
@samo38 samo38 deleted the us_archive branch October 9, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants