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.

Sign up for GitHub

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

Jump to bottom

Treat symlinks as normal files in spec file #901

Closed
wants to merge 1 commit into from
Closed

Treat symlinks as normal files in spec file #901

wants to merge 1 commit into from

Conversation

dpennell
Copy link
Contributor

Symlinks are now treated like normal files in the spec. All of the special handling in the scriptlets has been removed. Symlinks are now handled properly by upgrades.

@muuki88
Copy link
Contributor

muuki88 commented Oct 29, 2016

Thanks 😊 I'll take a closer when I come home tomorrow

@muuki88
Copy link
Contributor

muuki88 commented Oct 29, 2016

cc @fsat can you take a look? Would be nice 😊

@muuki88
Copy link
Contributor

muuki88 commented Oct 30, 2016 •
edited
Loading

@dpennell I would first merge #895 and release this. Sorry for the short answer above, I'm not back from vacation and can look into this in more detail.

@fsat implemented a great part of the initial relocation implementation, so it would be nice if he could validate your changes. Maybe he also tried this approach first, but didn't work out for him.

From my perspective I like the implementation. For two reasons

  1. Removing bash code ( always good )
  2. Staging directory looks more like the final system installation

@muuki88 muuki88 added the rpm label Oct 30, 2016
@dpennell
Copy link
Contributor Author

I am certainly not an RPM expert. I based my solution on what appeared to be well informed advice (see comments in #836) to just treat symlinks as regular files (which they are). I definitely think this needs a serious review. I've tested on my centos VM and it is being used in our product (currently in QA).

@muuki88 I think that this solution would remove the need for #895.

@muuki88
Copy link
Contributor

muuki88 commented Oct 31, 2016

Yes, I hope this will remove the need for #895.

I will test this my self, but I also would like to wait for @fsat review on this.
@huntc Conductr has used some of the relocation features as well, right?

muuki88
muuki88 approved these changes Nov 18, 2016
View reviewed changes
Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

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

@dpennell could you rebase this once more? I'll merge this as soon as the test a green.

I wasn't able to test this myself, but the implementation looks good to me.

src/sbt-test/rpm/symlink-rpm/build.sbt
val spec = IO.read(target / "rpm" / "SPECS" / "rpm-package.spec")
out.log.success(spec)
assert(spec contains "%attr(0644,root,root) /usr/share/rpm-package/lib/rpm-test.rpm-test-0.1.0.jar", "Wrong installation path\n" + spec)
assert(spec contains "/usr/share/rpm-package/lib/hello.link", "Missing or incorrect symbolic link\n" + spec)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong links appear in the spec like this?

@muuki88 muuki88 self-assigned this Nov 19, 2016
@muuki88 muuki88 mentioned this pull request Nov 19, 2016
Closed
@muuki88
Copy link
Contributor

muuki88 commented Nov 19, 2016

Closing this for #916 as I can't update this PR.

@muuki88 muuki88 closed this Nov 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muuki88 muuki88 muuki88 approved these changes

Assignees

@muuki88 muuki88

Labels
rpm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dpennell @muuki88