Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

Refactor schemaVersion to distinguish between 2 and 3, and drop support for 1 #531

Merged
merged 5 commits into from
Apr 5, 2018

Conversation

mseri
Copy link
Contributor

@mseri mseri commented Apr 5, 2018

This should solve #503 and #515.

This PR also introduces a way to delete a source, a patchqueue or a patch by providing an empty record in the link/pin file. I have created #529 to track that as I recall that issue being discussed at a certain point. I am happy to remove the relevant commits if we decide that this is not necessary.

EDIT: Removed, it will be part of a separate PR if we decide to stack pins over links

@mseri mseri requested review from euanh and srowe April 5, 2018 13:10
planex/spec.py Outdated
spec.add_archive(idx, value["URL"], link.path,
value["patches"])
if not value:
spec.rm_patch(idx)
Copy link
Contributor

@euanh euanh Apr 5, 2018

Choose a reason for hiding this comment

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

Why does rm_patch match with add_archive here (instead of rm_archive)? If that's what is intended could you please add a comment explaining why these two go together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed this comment. I replied below. I am happy to add a comment, but I wanted first to discuss it and decide if we want it at all.

planex/spec.py Outdated
for name, value in link.patch_sources.items():
idx = _parse_name(name)
if not value:
spec.rm_patch(idx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is rm_patch the opposite of add_archive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because archive is not really rewriting a patch, it's overriding where we get it from. Patch entries add the archive to override the patches, adding another patch allows us to override the archive, but the only way to delete the patch is to remove the patch entry.

Did I understand it incorrectly?

@euanh
Copy link
Contributor

euanh commented Apr 5, 2018

Removing sources could be dangerous. Sources are usually unpacked by specific macros in the %prep stanza, so removing a source completely (instead of overriding it) will break %prep.

Removing patches should be ok if %prep uses %autosetup or %autopatch, since those just iterate over all patches. Of course the remaining patches might not apply correctly afterwards...

I don't immediately see the use of removing archives and patchqueues, since those are only added by links so you could just not add them in the first place. I guess this could be useful if you were stacking a pin over a link, but at present the pin completely replaces the link.

@mseri
Copy link
Contributor Author

mseri commented Apr 5, 2018

Yes, I thought the idea was allow to stack a pin over a link. I think it's good to split this PR in two. One dealing with the link refactor (this PR) and a future one, if needed, that adds the removal feature. It is such a tiny amount of effor that it's not a big deal to rentroduce it later only if we find the necessity

@mseri mseri merged commit daf077d into xenserver:depend-refactor Apr 5, 2018
@mseri mseri deleted the link-rm branch April 5, 2018 19:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants