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

Replace YAML instructions with SPEC instructions #214

Merged
merged 3 commits into from Dec 19, 2022
Merged

Replace YAML instructions with SPEC instructions #214

merged 3 commits into from Dec 19, 2022

Conversation

vigejolla
Copy link
Member

No description provided.

@martyone
Copy link
Member

It's strange the diff is so big - after a bit closer glance it seems there may not actually be that much changes, it just fails to match the old and new content. It might help to preserve the section titles or move their changes into a separate commit - could you please try that? There may be other changes that, when split into a separate commit, would make the diff much smaller.

@vigejolla
Copy link
Member Author

It's strange the diff is so big - after a bit closer glance it seems there may not actually be that much changes, it just fails to match the old and new content. It might help to preserve the section titles or move their changes into a separate commit - could you please try that? There may be other changes that, when split into a separate commit, would make the diff much smaller.

Yeah, the section headings have changed, as they are really the keywords used in the files - spec files have different keywords than yaml files. The other thing which affects the large looking diff is the order of paragraps, as I had to move things around a bit.

I'll try to separate these into separate commits, let's see...

@vigejolla vigejolla force-pushed the jb59289 branch 2 times, most recently from 7d475a9 to 3da4f31 Compare December 19, 2022 07:22
@vigejolla
Copy link
Member Author

It should be a bit easier to review now. Please note that this splitting of the commit was done solely for the purpose of the review, I intend to squash the commits before merging.

@martyone
Copy link
Member

hm, did it really go as intended? To me it seems the first commit does some moves that the second commit reverts.

I would definitely not squash the commits - atomic commits are very valuable.

@vigejolla
Copy link
Member Author

🤦 let me try again...

@vigejolla
Copy link
Member Author

Check now

Copy link
Member

@martyone martyone left a comment

Choose a reason for hiding this comment

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

Looks better now.

Wouldn't it be more natural to list BuildRequires before Requires? I see that our spectacle does it the opposite way but e.g. here https://rpm-packaging-guide.github.io/ you can see BuildRequires first. This would save us from some unnecessary shuffling of the sections too. Leave the Description on its original place as well and all we need to move is the "About the PkgConfigBR and PkgBR Keywords" section :)

It would be nice to have a different type of arrow from .spec to .rpm in the diagram. The relation is quite different from the one between .pro and .spec.

Please do NOT squash the commits.

@vigejolla
Copy link
Member Author

Wouldn't it be more natural to list BuildRequires before Requires? I see that our spectacle does it the opposite way but e.g. here https://rpm-packaging-guide.github.io/ you can see BuildRequires first. This would save us from some unnecessary shuffling of the sections too. Leave the Description on its original place as well and all we need to move is the "About the PkgConfigBR and PkgBR Keywords" section :)

It would. We don't use spectacle anymore, so we could change the order in the templates as well. That's a minor detail, but I'll file another task about that. And change the order in the document already in this PR.

It would be nice to have a different type of arrow from .spec to .rpm in the diagram. The relation is quite different from the one between .pro and .spec.

I agree. I just tried to mimic the original diagram. I'll change the arrow type.

Please do NOT squash the commits.

Ok, ok :)

Signed-off-by: Ville Nummela <ville.nummela@jolla.com>
Signed-off-by: Ville Nummela <ville.nummela@jolla.com>
Signed-off-by: Ville Nummela <ville.nummela@jolla.com>
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