Skip to content

Fix symbolic link handling in pelican-themes -s/-c#3392

Merged
justinmayer merged 2 commits into
getpelican:mainfrom
frederik-elwert:fix-symlinks
Sep 28, 2024
Merged

Fix symbolic link handling in pelican-themes -s/-c#3392
justinmayer merged 2 commits into
getpelican:mainfrom
frederik-elwert:fix-symlinks

Conversation

@frederik-elwert

Copy link
Copy Markdown
Contributor

Pull Request Checklist

  • Ensured tests pass and (if applicable) updated functional test output
  • Conformed to code style guidelines by running appropriate linting tools
  • Added tests for changed code
  • Updated documentation for changed code

Description

Symlinks with relative paths were not handled properly by pelican-themes.

Example:

The pelican theme to be installed is in a sibling directory to the project directory. Running

pelican-themes -s ../my-theme

creates a symlink ../my-theme in pelican’s theme folder. This link is broken, since the relative path in the symlink will now be resolved from pelican’s theme folder.

When running pelican-themes -c from the project folder, it will not detect the broken symlink, since the relative path will actually resolve from the current directory (just not from pelican’s theme folder).

This pull request fixes this by running os.path.realpath on the passed path both when creating symlinks and when checking symlinks. This will make sure that always the absolute paths are linked/checked.

@justinmayer

Copy link
Copy Markdown
Member

@frederik-elwert: Thank you for the submission!

Any thoughts from @getpelican/reviewers regarding this pull request?

@boxydog

boxydog commented Sep 21, 2024

Copy link
Copy Markdown
Contributor

Thanks for the contribution.

It would be nice to have a unit test. It might be pretty easy. Whatever broke before.

@frederik-elwert

Copy link
Copy Markdown
Contributor Author

Thanks for the contribution.

It would be nice to have a unit test. It might be pretty easy. Whatever broke before.

Yes, I agree, it would always be good to have tests. I'm not super experienced with writing tests, and pelican-themes requires file system level interactions with the current Python environment, so I was a bit out of my depth.

@justinmayer justinmayer changed the title Fix symbolic link handling in pelican-themes -s/-c Fix symbolic link handling in pelican-themes -s/-c Sep 28, 2024

@justinmayer justinmayer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Many thanks for the fix, Frederik. I'm going to merge this now so that we can release a fix for this and thereby eliminate the problem for end users as soon as possible. If you or @boxydog can think of one or more unit tests that might prevent future regressions, submitting them in a follow-up pull request would be most appreciated. Thanks again!

@justinmayer justinmayer merged commit 08283b4 into getpelican:main Sep 28, 2024
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.

4 participants