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 test-sos as a submodule #1106

Merged
merged 22 commits into from
Feb 16, 2024

Conversation

kholland-intel
Copy link
Collaborator

No description provided.

autogen.sh Outdated Show resolved Hide resolved
Also, updated the github actions/checkout version to v4
autogen.sh Outdated Show resolved Hide resolved
@davidozog
Copy link
Member

Let's add --recursive-submodule to the example build scripts.

davidozog
davidozog previously approved these changes Dec 11, 2023
Copy link
Member

@davidozog davidozog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Suggest adding a comment in the RPM build in CI.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
autogen.sh Outdated Show resolved Hide resolved
@wrrobin
Copy link
Collaborator

wrrobin commented Dec 13, 2023

I did not try it out, but do we have to use recurse-submodule now every time? Do we need any update on our README or wiki pages?

@davidozog
Copy link
Member

I did not try it out, but do we have to use recurse-submodule now every time? Do we need any update on our README or wiki pages?

Yes, if you want to build SOS successfully you will now need to call git clone --recurse-submodules <SOS> to clone both SOS and the tests-sos submodule. You could alternatively clone normally (i.e. without --recurse-submodules) then run git submodule update --init from inside the repo before running autogen.sh.

I think we should also consider a feature where SOS can be built without having to clone and build the tests, but that is not supported in this PR. Do you think we should do that before merging this change?

I absolutely think we should document everything before merging. Also please note my comment above about testing any PR that changes both SOS and the tests-sos submodule... This procedure could be confusing without documentation.

@davidozog davidozog mentioned this pull request Jan 31, 2024
@wrrobin
Copy link
Collaborator

wrrobin commented Feb 1, 2024

@davidozog - I was trying this branch out and the submodule download always getting stuck for me. Not sure if the issue is related with git config. Did you see this?

$> git clone -b "pr/test-submodule" --recurse-submodules https://github.com/kholland-intel/SOS.git sos-submodule
Cloning into 'sos-submodule'...
remote: Enumerating objects: 17958, done.
remote: Counting objects: 100% (1362/1362), done.
remote: Compressing objects: 100% (499/499), done.
remote: Total 17958 (delta 930), reused 1162 (delta 825), pack-reused 16596
Receiving objects: 100% (17958/17958), 5.33 MiB | 18.31 MiB/s, done.
Resolving deltas: 100% (13463/13463), done.
Updating files: 100% (1806/1806), done.
Submodule 'modules/tests-sos' ([email protected]:openshmem-org/tests-sos.git) registered for path 'modules/tests-sos'
Cloning into '/mnt/scratch/rahmanmd/sos-submodule/modules/tests-sos'...

Quick google search led to https://stackoverflow.com/questions/40841882/automatically-access-git-submodules-via-ssh-or-https/44630028#44630028 which suggests to use relative path for submodules. Not sure if that is the fix.

@davidozog
Copy link
Member

davidozog commented Feb 7, 2024

@wrrobin @philipmarshall21 - It seems like we're spending too much time waiting on the UCX with pmi-mpi test row. I propose we remove it or swap it out with something else until #1048 is resolved.

Is the UCX with pmi-mpi test case important to anybody listening? @jdinan perhaps? It's not needed on our end...

@wrrobin
Copy link
Collaborator

wrrobin commented Feb 7, 2024

@davidozog - The changes resolved the issue of make check.

@davidozog davidozog dismissed their stale review February 8, 2024 16:52

PR is now assigned to me, so dismissing my review from earlier

Copy link
Collaborator

@wrrobin wrrobin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with different build directories. Works perfectly.

@davidozog
Copy link
Member

Tested with different build directories. Works perfectly.

This reminds me we need CI to cover the case of users building in the SOS root directory - added issue #1111 so we can merge this asap since it's holding up other PRs.

@davidozog davidozog merged commit 12cd72b into Sandia-OpenSHMEM:main Feb 16, 2024
34 checks passed
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.

4 participants