Skip to content

Ensure posix paths in manifest #425

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

Merged
merged 4 commits into from
May 25, 2023
Merged

Ensure posix paths in manifest #425

merged 4 commits into from
May 25, 2023

Conversation

mmarchetti
Copy link
Contributor

This PR ensures that manifest file lists always contain posix paths, and not Windows paths (containing \). shinyapps.io does not accept Windows paths in the manifest.json.

Intent

Fixes #373

Type of Change

  • Bug Fix
  • New Feature
  • Breaking Change

Approach

Using pathlib.Path.as_posix() to convert to Posix paths.

Automated Tests

I removed all of the special case unit tests for Windows, and make all of the tests run on all platforms.

Directions for Reviewers

Will need to test deployments on posix and Windows clients, deploying to Connect and to shinyapps.io.

Checklist

  • I have updated CHANGELOG.md to cover notable changes.
  • I have updated all related GitHub issues to reflect their current state.

@github-actions
Copy link

github-actions bot commented May 19, 2023

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
4197 2654 63% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
rsconnect/bundle.py 77% 🟢
TOTAL 77% 🟢

updated for commit: 8774a3d by action🐍

@mmarchetti mmarchetti requested a review from bcwu May 19, 2023 19:06
@jcheng5
Copy link
Contributor

jcheng5 commented May 24, 2023

Hi @mmarchetti, gentle nudge. Anything standing in the way of this getting merged and released? Let me know if I can help.

@kgartland-rstudio
Copy link
Contributor

kgartland-rstudio commented May 24, 2023

Hi @mmarchetti, gentle nudge. Anything standing in the way of this getting merged and released? Let me know if I can help.

I'm testing this now and the manifests created in Windows now look to be using the proper filepaths, but is there a live shinyapps.io instance where I can deploy to ensure they're working properly?

EDIT:
@dskard helped me out. I'm testing deploys now.

@kgartland-rstudio
Copy link
Contributor

Test app with nested directories:
https://huggingface.co/spaces/posit/shiny-for-python-template/tree/main

shinyapps.io

  • write-manifest and deploy from windows
  • write-manifest and deploy from macOS
  • write-manifest and deploy from linux

connect

  • write-manifest and deploy from windows
  • write-manifest and deploy from macOS
  • write-manifest and deploy from linux

Also deployed other manifest content and ran rsconnect-python tests in Connect against this branch. All passed.

Good to merge!

@mmarchetti mmarchetti merged commit 6a36b39 into master May 25, 2023
@mmarchetti mmarchetti deleted the mm-posix-paths branch May 25, 2023 12:16
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.

Shiny app deployment fails when static content is present in the app
4 participants