Make git upstream act more like tar #22
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit ca32b1e. Evidently this wasn't working correctly on Windows. Signed-off-by: Scott K Logan <[email protected]>
cottsay
added
bug
clalancette
requested changes
Feb 1, 2021
cottsay
force-pushed
the
cottsay/no_extra_update
branch
from
dc42d53 to
7a90b62
Compare
clalancette
approved these changes
Feb 1, 2021
cottsay
force-pushed
the
cottsay/no_extra_update
branch
from
7a90b62 to
3af6812
Compare
clalancette
approved these changes
Feb 1, 2021
| GIT_REPOSITORY https://github.com/uncrustify/uncrustify.git | ||
| GIT_TAG ${uncrustify_version} | ||
| GIT_CONFIG advice.detachedHead=false | ||
| # Suppress git update due to https://gitlab.kitware.com/cmake/cmake/-/issues/16419 | ||
| # See https://github.com/ament/uncrustify_vendor/pull/22 for details | ||
| UPDATE_COMMAND "" |
There was a problem hiding this comment.
As we just found out, https://github.com/eProsima/foonathan_memory_vendor/blob/0e6174a7f28c45faf711030697a6c32547336acd/CMakeLists.txt#L79 also has this workaround. So 🤞 this is the right solution.
Some of our problems with using GIT_REPOSITORY with a GIT_TAG stem from CMake trying to "update" the repository each time the target is evaluated. This would be important if not for the fact that we're targeting a specific REF, so the repository will never be out-of-date unless we change the ref, which would invalidate the target anyway. We can safely use UPDATE_DISCONNECTED to block the update step from invalidating the source target, which should prevent downstream targets like patch, build, and install from being re-triggered unnecessarily. The one caveat to this is that a change to GIT_TAG doesn't invalidate the target like a change to GIT_REPOSITORY or URL would. To work around this, I updated the target name to contain the ref instead of the supposed version number, which will then force a new build if we change the ref in the future. This is also more correct, since it would be easy for the ref and target name to get out of sync. Signed-off-by: Scott K Logan <[email protected]>
This reverts commit c614081. This is no longer necessary because of UDPATE_DISCONNECTED, which will make the download step act more like using a URL instead of a GIT_REPOSITORY. Signed-off-by: Scott K Logan <[email protected]>
cottsay
force-pushed
the
cottsay/no_extra_update
branch
from
3af6812 to
8b08698
Compare
|
The last force-push was just to update the commit message to prepare for a rebase-merge to preserve the revert history. |
cottsay
added a commit
that referenced
this pull request
Feb 1, 2021
Some of our problems with using GIT_REPOSITORY with a GIT_TAG stem from CMake trying to "update" the repository each time the target is evaluated. This would be important if not for the fact that we're targeting a specific REF, so the repository will never be out-of-date unless we change the ref, which would invalidate the target anyway. We can safely use UPDATE_DISCONNECTED to block the update step from invalidating the source target, which should prevent downstream targets like patch, build, and install from being re-triggered unnecessarily. The one caveat to this is that a change to GIT_TAG doesn't invalidate the target like a change to GIT_REPOSITORY or URL would. To work around this, I updated the target name to contain the ref instead of the supposed version number, which will then force a new build if we change the ref in the future. This is also more correct, since it would be easy for the ref and target name to get out of sync. Signed-off-by: Scott K Logan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This package is proving to be a perfect storm of the following two issues:
Issue (1) initially caused problems for this package because it has a patching step. We mitigated that in #20 by essentially reverting any changes prior to applying the patch. So (1) is still manifesting in these builds, we just made the patch step robust in the face of an unclean source tree.
Issue (2) actually affects all of our vendor packages, but we normally don't see it because everywhere we're using
DESTDIRwe're also using discrete build and install invocations. However, due to this package being affected by (1), the external package got rebuilt during the install phase andDESTDIRmessed with the installation into the staging directory.Rather than making our patch step more robust, this change suppresses the "update" step for the external package, which is why the patch step was getting re-run in the first place. Without this update step, the package will act much more like it did when we used a
URLupstream instead ofGIT_REPOSITORY.The one caveat is that a change to
GIT_TAGdoesn't seem to invalidate the target like changingURLorGIT_REPOSITORYwould. To mitigate that, I changed the entire target's name to include the ref, which is probably more accurate anyway.This changes reverts #20 because it is no longer needed in this package any more than it might be needed in a
URL-based package.This change also reverts #21 because it was causing problems on Windows, and the package should no longer misbehave when faced with
DESTDIRas long as discrete build and install phases are used properly.Release:
Debug: