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

Build: don't care about the filename for the offline formats #10873

Closed
humitos opened this issue Oct 26, 2023 · 0 comments · Fixed by #11198
Closed

Build: don't care about the filename for the offline formats #10873

humitos opened this issue Oct 26, 2023 · 0 comments · Fixed by #11198
Assignees
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code

Comments

@humitos
Copy link
Member

humitos commented Oct 26, 2023

Projects using build.commands to build their documentation can make usage of the $READTHEDOCS_OUTPUT directory to expose offline formats like PDF, ePUB, HTMLZip as documented in https://docs.readthedocs.io/en/latest/build-customization.html

Currently, we support one and only one file per offline format. We have an in-progress work at #10438, but the work required there is not trivial and it may be off from our roadmap/prioritization for some time now. I think that feature is important for big projects like CPython documentation and we should work on that as soon as possible, tho.

In the meantime, we could make the build process a little simpler by allowing any filename for the PDF saved under $READTHEDOCS_OUTPUT/pdf/ since right now it requires to exactly be $READTHEDOCS_OUTPUT/pdf/$READTHEDOCS_PROJECT.pdf --which is not documented anywhere 😄

I'd say that, as long as there is one and only one PDF file inside the output directory, we shouldn't care about the filename and rename it internally (probably at upload time) to be what we need it to be.

Code references:

  • if artifact_type in ARTIFACT_TYPES_WITHOUT_MULTIPLE_FILES_SUPPORT:
    artifact_format_files = len(os.listdir(artifact_directory))
    if artifact_format_files > 1:
    log.error(
    "Multiple files are not supported for this format. "
    "Skipping this output format.",
    output_format=artifact_type,
    )
    raise BuildUserError(
    BuildUserError.BUILD_OUTPUT_HAS_MULTIPLE_FILES.format(
    artifact_type=artifact_type
    )
    )
    if artifact_format_files == 0:
    raise BuildUserError(
    BuildUserError.BUILD_OUTPUT_HAS_0_FILES.format(
    artifact_type=artifact_type
    )
    )
  • # Upload formats
    for media_type in types_to_copy:
    from_path = self.data.project.artifact_path(
    version=self.data.version.slug,
    type_=media_type,
    )
    to_path = self.data.project.get_storage_path(
    type_=media_type,
    version_slug=self.data.version.slug,
    include_file=False,
    version_type=self.data.version.type,
    )
    self._log_directory_size(from_path, media_type)
    try:
    build_media_storage.rclone_sync_directory(from_path, to_path)
    except Exception as exc:
    # NOTE: the exceptions reported so far are:
    # - botocore.exceptions:HTTPClientError
    # - botocore.exceptions:ClientError
    # - readthedocs.doc_builder.exceptions:BuildCancelled
    log.exception(
    "Error copying to storage",
    media_type=media_type,
    from_path=from_path,
    to_path=to_path,
    )
    # Re-raise the exception to fail the build and handle it
    # automatically at `on_failure`.
    # It will clearly communicate the error to the user.
    raise BuildAppError("Error uploading files to the storage.") from exc
@humitos humitos added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Oct 26, 2023
humitos added a commit that referenced this issue Mar 8, 2024
Currently, we _require_ the user to save the PDF or ePUB file with a pretty
specific filename: `{project.slug}-{version.slug}.{extension}`. This has caused
a lot of confusions to users.

Since we only support 1 file for PDF/ePUB for now, we can rename this file to
the valid filename automatically.

Closes #10873
humitos added a commit that referenced this issue Jul 11, 2024
* Build: rename PDF/ePUB filename to valid one automatically

Currently, we _require_ the user to save the PDF or ePUB file with a pretty
specific filename: `{project.slug}-{version.slug}.{extension}`. This has caused
a lot of confusions to users.

Since we only support 1 file for PDF/ePUB for now, we can rename this file to
the valid filename automatically.

Closes #10873

* Rename the offline format file properly

* Initial attempt to write a test case

* Revert "Initial attempt to write a test case"

This reverts commit 76ab170.

* Test case and filename fix

* Remove the conditional that's not needed

* Assert the path is inside OUTPUT dir before calling `shutil.move`

* Revert "Assert the path is inside OUTPUT dir before calling `shutil.move`"

This reverts commit e9a0f08.

* Assert the path is inside the DOCROOT before calling `shutil.move`

* Update readthedocs/projects/tasks/builds.py

Co-authored-by: Santos Gallegos <[email protected]>

---------

Co-authored-by: Santos Gallegos <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant