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

Force shallow clone of json 3-rd party #12914

Merged
merged 2 commits into from May 12, 2024

Conversation

Nir-Az
Copy link
Collaborator

@Nir-Az Nir-Az commented May 9, 2024

CMake ignore the shallow clone, json history is huge (185 [MB] when the repo itself is ~7 [MB]) and take much time to download.
Jenkins fail on that many times on timeout.
This fix it.

Before:
image
image

After:
image
image

Also notified json on GH
nlohmann/json#4370

@Nir-Az Nir-Az marked this pull request as ready for review May 9, 2024 14:01
@Nir-Az Nir-Az requested a review from maloel May 9, 2024 15:01
@maloel
Copy link
Collaborator

maloel commented May 10, 2024

CMake ignore the shallow clone

Source? It's supposed to work as of v3.6, no?

# We do not use 'GIT_REPOSITORY' as it doesn't support a shallow clone of a specific commit,
# this make the clone step very long, so we clone by ourselves
DOWNLOAD_COMMAND git clone -c advice.detachedHead=false --branch v3.11.3 https://github.com/nlohmann/json.git --depth 1 json
DOWNLOAD_DIR ${CMAKE_BINARY_DIR}/third-party/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think better to keep the quotes in case the binary dir has spaces in it - though I'm not sure it matters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I try or keep it as is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think quotes are better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@Nir-Az
Copy link
Collaborator Author

Nir-Az commented May 10, 2024

CMake ignore the shallow clone

Source? It's supposed to work as of v3.6, no?

Its not working , see LibCI which use > 3.20
Tried it also locally

@maloel
Copy link
Collaborator

maloel commented May 10, 2024

CMake ignore the shallow clone

Source? It's supposed to work as of v3.6, no?

Its not working , see LibCI which use > 3.20 Tried it also locally

That's not in doubt -- I believe you.
But I'd still like to see a source that says this is a known issue and how to get around it...

@Nir-Az
Copy link
Collaborator Author

Nir-Az commented May 10, 2024

CMake ignore the shallow clone

Source? It's supposed to work as of v3.6, no?

Its not working , see LibCI which use > 3.20 Tried it also locally

That's not in doubt -- I believe you. But I'd still like to see a source that says this is a known issue and how to get around it...

https://gitlab.kitware.com/cmake/cmake/-/issues/17770

@maloel
Copy link
Collaborator

maloel commented May 12, 2024

https://gitlab.kitware.com/cmake/cmake/-/issues/17770

Great, thanks. It says:

For some git versions [>=1.7.10] the --no-single-branch option is passed, and this option will cause all branch HEAD's to be downloaded, instead of just the branch/tag/commit that one wanted to download

So we're trying to take out this flag.

Couple of things:

  • Sounds like we need a general function git_shallow_clone that we would use here and in many other places, right? This is not a weird repo where this problem exists: someone else may as well complain to us that our repo is too big. Can we refactor to an actual function that we document and call instead of replicating this change everywhere? (possibly in another PR, up to you)
  • This may be a problem with ExternalProject_Add, but another option would be to use FetchContent, no?
    • We haven't so far because it's not available before CMake 3.11, but now that we're removing U18, our minimum CMake version can be increased too?
    • Maybe FetchContent is the reason this was never fixed it in ExternalProject_Add?

Jenkins fail on that many times on timeout.

I have yet to see this... but agree that this is better if it indeed helps.

@Nir-Az
Copy link
Collaborator Author

Nir-Az commented May 12, 2024

I agree it will be helpful to deep dive on this, I will open a ticket for it.
I suggest we merge this one as it's causing many issues and try to work on a more generic approach.

@Nir-Az
Copy link
Collaborator Author

Nir-Az commented May 12, 2024

https://gitlab.kitware.com/cmake/cmake/-/issues/17770

Great, thanks. It says:

For some git versions [>=1.7.10] the --no-single-branch option is passed, and this option will cause all branch HEAD's to be downloaded, instead of just the branch/tag/commit that one wanted to download

So we're trying to take out this flag.

Couple of things:

  • Sounds like we need a general function git_shallow_clone that we would use here and in many other places, right? This is not a weird repo where this problem exists: someone else may as well complain to us that our repo is too big. Can we refactor to an actual function that we document and call instead of replicating this change everywhere? (possibly in another PR, up to you)

  • This may be a problem with ExternalProject_Add, but another option would be to use FetchContent, no?

    • We haven't so far because it's not available before CMake 3.11, but now that we're removing U18, our minimum CMake version can be increased too?
    • Maybe FetchContent is the reason this was never fixed it in ExternalProject_Add?

Jenkins fail on that many times on timeout.

I have yet to see this... but agree that this is better if it indeed helps.

BTW, here you did a lot of work to make it run on cmake configure step (call it from a different file), it is also relevant for a full external project step which also compile (like in libcurl)
So a generic function is complicated to do for all cases.
We need to think about it

SOURCE_DIR "${CMAKE_BINARY_DIR}/third-party/json"
GIT_SHALLOW 1 # No history needed (requires cmake 3.6)

# We do not use 'GIT_REPOSITORY' as it doesn't support a shallow clone of a specific commit,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please point to the article you pointed to in the PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

And also explain advice.detachedHead=false

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@Nir-Az Nir-Az merged commit 8524975 into IntelRealSense:development May 12, 2024
17 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.

None yet

2 participants