Add --enable-rojig, disable by default #2278
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
|
/lgtm |
jlebon
reviewed
Oct 22, 2020
tests/vmcheck/test-rojig-client.sh
Outdated
| @@ -30,6 +30,12 @@ if test "${osid}" != 'ID=fedora'; then | |||
| exit 0 | |||
| fi | |||
|
|
|||
| $(vm_rpmostree --version) > version.txt | |||
There was a problem hiding this comment.
Note this will still allocate a whole VM. We should probably enhance filter_tests() in a follow-up to just skip over tests with rojig in the name if it's compiled out.
We're seeing some CI failures that I think are a bug in rojig. In the bigger picture...we never actually started using this, and I think longer term shipping os updates via containers probably makes more sense. I put a *lot* of effort into this code and it's pretty cool so it's hard to just delete it. And *maybe* someone out there is using it (but I doubt it). So rather than just deleting it entirely let's make it a build-time option. I verified that it builds at least.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, jlebon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cgwalters
added a commit
to cgwalters/rpm-ostree
that referenced
this pull request
May 18, 2021
The inevitable followup to coreos#2278 that I was too cowardly to do at the time. But it's time to admit the 2 months or so of work on this was wasted. We have too much tech debt and this is a large chunk of C/C++ code that touches everything in the codebase in a nontrivial way. Bigger picture, I'm going to work on coreos/fedora-coreos-tracker#828 which will strongly orient rpm-ostree towards the container world instead. We'll still obviously keep the rpm package world around, but only as a secondary layer. What rojig was trying to do in putting "images" inside an RPM was conflating layers. It would have had a lot of benefits probably if we'd truly pushed it over the edge into completion, but that didn't happen. Let's focus on containers instead. There's still a lot more rojig code to delete but this first patch removes the bulk of it. Touching everything that references e.g. `RPMOSTREE_REFSPEC_TYPE_ROJIG` etc. can come as a 3rd phase.
cgwalters
added a commit
to cgwalters/rpm-ostree
that referenced
this pull request
May 18, 2021
The inevitable followup to coreos#2278 that I was too cowardly to do at the time. But it's time to admit the 2 months or so of work on this was wasted. We have too much tech debt and this is a large chunk of C/C++ code that touches everything in the codebase in a nontrivial way. Bigger picture, I'm going to work on coreos/fedora-coreos-tracker#828 which will strongly orient rpm-ostree towards the container world instead. We'll still obviously keep the rpm package world around, but only as a secondary layer. What rojig was trying to do in putting "images" inside an RPM was conflating layers. It would have had a lot of benefits probably if we'd truly pushed it over the edge into completion, but that didn't happen. Let's focus on containers instead. There's still a lot more rojig code to delete but this first patch removes the bulk of it. Touching everything that references e.g. `RPMOSTREE_REFSPEC_TYPE_ROJIG` etc. can come as a 3rd phase.
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.
We're seeing some CI failures that I think are a bug in rojig.
In the bigger picture...we never actually started using this,
and I think longer term shipping os updates via containers
probably makes more sense.
I put a lot of effort into this code and it's pretty cool
so it's hard to just delete it. And maybe someone out there
is using it (but I doubt it). So rather than just deleting
it entirely let's make it a build-time option.
I verified that it builds at least.