Skip to content

Conversation

@SaravananSathyanandhaQC
Copy link
Contributor

@SaravananSathyanandhaQC SaravananSathyanandhaQC commented Feb 13, 2024

Removes a patch as per #33

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@mfisher87
Copy link
Member

@SaravananSathyanandhaQC would you like to be added as a maintainer?

@mfisher87
Copy link
Member

I haven't seen this before, I think this is new now that we're trying to build 1.4.

It looks like copyPandocScript function may need to be updated to ensure the target directory exists before making the symbolic link. I'm not 100% sure of that and I've forgotten some of my bag of tricks; it's been a while since I've built a conda package :) If that is the problem, we can patch, but I'd like to get upstream involved if the fix is that simple. We can probably get a quick turnaround on an upstream PR and avoid a patch 🚀

Are you on the Conda Forge Matrix server? We could chat there to work through some of the issues in a more real-time way if you'd like!

@mfisher87
Copy link
Member

💭 While we're in here, should we consider updating these patches so they don't require offsets or fuzzing?

[[ RA-MD1LOVE ]] - [[                             patches/0001-win-conda-path-overrides.patch ]]
[[ RA-MD1L-VE ]] - [[                              patches/0001-log-deno-bundle-command.patch ]]
[[ RA-MD1L--E ]] - [[             patches/0001-remove-QUARTO_VERSION-from-configuration.patch ]]

@mfisher87
Copy link
Member

mfisher87 commented Feb 13, 2024

This patch gets me past the error, but now I get a warning.

diff --git a/package/src/common/configure.ts b/package/src/common/configure.ts
index e5f83603e..a77316b26 100644
--- a/package/src/common/configure.ts
+++ b/package/src/common/configure.ts
@@ -154,7 +154,9 @@ export function copyPandocScript(config: Configuration, targetDir: string) {
   }
 
   if (Deno.build.os !== "windows") {
-    info("> creating pandoc symlink");
+    info("> creating pandoc symlink in " + targetDir);
+    Deno.run({cmd: ["mkdir", "-p", targetDir]});  
     Deno.run({
       cwd: targetDir,
       cmd: ["ln", "-s", linkTarget, "pandoc"]

Warning:

/opt/conda/lib/python3.10/site-packages/conda_build/build.py:1672: UserWarning: file /home/conda/feedstock_root/build_artifacts/quarto_1707866209788/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_plac/bin/tools/pandoc is a symlink with no target
  warnings.warn("file %s is a symlink with no target" % path, UserWarning)

@mfisher87
Copy link
Member

mfisher87 commented Feb 13, 2024

I seem to have also gotten a build without that warning, but not sure how. After installing that locally-built package, I can do quarto create-project just fine, but quarto preview prints a bunch of these:

WARNING: No such file or directory (os error 2): readfile '/path/to/miniconda3/envs/quarto-1.4-test/share/quarto/preview/quarto-preview.js'

Indeed, no such file is there. Although it does actually render a website!

I have to set this down for now, but hopefully the above was helpful :)

@mfisher87
Copy link
Member

mfisher87 commented Feb 13, 2024

@cderv any thoughts on the symlink issue we're seeing and upstreaming the patch?

@dragonstyle
Copy link

dragonstyle commented Feb 14, 2024

Are the dependencies for Quarto being downloaded as a part of configure in this case? If not, that is likely the source of the issue as that portion of the configuration is expecting pandoc to have been downloaded and placed in that directory. If that isn't possible in this case, we could consider just making that WARN that the symlink couldn't be created or as you note, just creating the directory.

edit: I'd be in favor of upstreaming whatever you need so you don't have to keep patching Quarto!

@mfisher87
Copy link
Member

mfisher87 commented Feb 15, 2024

edit: I'd be in favor of upstreaming whatever you need so you don't have to keep patching Quarto!

Awesome, thanks :)

Are the dependencies for Quarto being downloaded as a part of configure in this case? If not, that is likely the source of the issue as that portion of the configuration is expecting pandoc to have been downloaded and placed in that directory.

I'm not completely sure! I've been learning as I go and the configuration process has been a bit mystifying to me. Apologies for needing my hand held a little bit here :) Is there any documentation on the configuration process you could point me to, or if not, perhaps we could collaborate to make some?

I see from the following from my test build:

  • configure.sh is downloading Deno Stdlib, then calling quarto-bld configure:
    • quarto-bld configure outputs the following. From reading the code (dependencies.ts), if these were downloaded, a log would have been written to indicate that (info(`Downloading ${dependency.name}`);), but log entry is not present:
      Preparing deno_dom (linux - x86_64)
      Configuring deno_dom
      deno_dom complete.
      
      Preparing Pandoc (linux - x86_64)
      Configuring Pandoc
      Pandoc complete.
      
      Preparing Dart Sass Compiler (linux - x86_64)
      Configuring Dart Sass Compiler
      Dart Sass Compiler complete.
      
      Preparing esbuild javscript bundler (linux - x86_64)
      Configuring esbuild javscript bundler
      esbuild javscript bundler complete.
      
      Preparing Typst (linux - x86_64)
      Configuring Typst
      Typst complete.
    • Should this step have failed ☝️ ? Have we misconfigured something that causes the downloads to not occur?
  • The build continues and eventually completes successfully.
    • In the built env, we have a bin/tools/pandoc symlink pointing at x86_64/pandoc; however, that symlink is the only thing in bin/tools, so the symlink points to nothing!
    • If I do which pandoc, pandoc is present in my env at bin/pandoc, which is not what the symlink expects. How did that happen?

@dragonstyle
Copy link

dragonstyle commented Feb 15, 2024

Ok, I think I see what is happening (and you are essentially correct in your comment above). Typically, when we configure Quarto, we place all the various binary dependencies within the bin directory (you'd see progress messages indicating that we are downloading the various dependencies and emplacing them). In this recipe, however, QUARTO_VENDOR_BINARIES=false instructs us not to emplace the binaries, so we essentially skip placing the various dependencies in place.

https://github.com/quarto-dev/quarto-cli/blob/4cc168d3806d36437ec36b32fc924b361df31753/package/src/common/dependencies/dependencies.ts#L74

Because we never place pandoc, the symlink fails.

I think the right fix is to respect the vendoring flag when attempting to make that symlink as well. Does that make sense?

dragonstyle added a commit to quarto-dev/quarto-cli that referenced this pull request Feb 15, 2024
Note that if we are being instructed not to vendor binaries,
Pandoc won't be present in the architecture specific directory, so
just skip this step.

See conda-forge/quarto-feedstock#36
@dragonstyle
Copy link

Ok, I've made the upstream fix here:

quarto-dev/quarto-cli@7526406

this just missed a patch release of Quarto 1.4, so is in our pre-release builds of Quarto 1.5.

For the time being, I think it may make sense to just use that commit to patch 1.4.449 (or .450) until we release our next path release (I can back port this to our 1.4 branch)- does that work?

@mfisher87
Copy link
Member

Right on @dragonstyle, thank you for your expertise here!

I will try and do a test build with a patch from the commit you linked. I'll include information about the expected lifetime of the patch in the patch message, and link back to your comment above so future us can get back on the same page :)

May be a bit later tonight, or even tomorrow. Not sure yet :)

@mfisher87
Copy link
Member

Got a successful build with this change, commit pushing shortly. However, when I test, I still get so much spam when I run quarto preview:

WARNING: No such file or directory (os error 2): readfile '/home/robatt/.local/share/miniconda3/envs/quarto-1.4-test/share/quarto/preview/quarto-preview.js'

Every time a page loads, dozens of these print. But in the browser, everything seems to be working great.

@mfisher87
Copy link
Member

I was able to get this to build with this heavy-handed patch: e86cb4b

I'm not sure how / if this could be addressed upstream. @dragonstyle thoughts?

@dragonstyle
Copy link

That's a reasonable patch for the time being - I will make an upstream change that will probably just force rebuilding it when configure is run, but that will have the same issue as the other upstream fix (need to keep the patch for now).

That JS handles auto-reload of previews (e.g. run quarto preview, then modify the qmd and ensure reload happens), just FYI.

@mfisher87
Copy link
Member

Sounds great. I'll try building a couple more of my projects and then I think we can merge this.

@mfisher87 mfisher87 marked this pull request as ready for review February 16, 2024 00:23
@mfisher87
Copy link
Member

I was able to build ~5 different projects, some using Jupyter, some using LaTex, some RevealJS. Let's go!

@mfisher87
Copy link
Member

mfisher87 commented Feb 16, 2024

OSX build had a timeout downloading mamba forge. I think we need some magic incantations to either update the feedstock or just re-run.

@mfisher87
Copy link
Member

@conda-forge-admin, please rerender

@github-actions
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/quarto-feedstock/actions/runs/7924564999.

@mfisher87
Copy link
Member

@conda-forge-admin, please restart ci

@mfisher87
Copy link
Member

I'm not sure why some of the builds aren't being restarted. I'm going to wait it out and then restart CI again and see if that works. 😩

@mfisher87 mfisher87 merged commit 2206b72 into conda-forge:main Feb 16, 2024
dragonstyle added a commit to quarto-dev/quarto-cli that referenced this pull request Feb 16, 2024
Note that if we are being instructed not to vendor binaries,
Pandoc won't be present in the architecture specific directory, so
just skip this step.

See conda-forge/quarto-feedstock#36
@dragonstyle
Copy link

(Upstream fix to preview issue here:

quarto-dev/quarto-cli@78ae90e )

Both fixes are back ported and will be in our next patch release of 1.4 (no ETA at this time)

Thank you for all your work on this @mfisher87 !!!

@mfisher87
Copy link
Member

Amazing! Same to you @dragonstyle , pleasure working with you :)

@cderv
Copy link
Contributor

cderv commented Feb 19, 2024

hey @mfisher87 !

Sorry I was off past week. Thanks a lot for the all the work, and thanks @dragonstyle for helping with this !

Awesome !

@mfisher87
Copy link
Member

❤️ Welcome back, hope you had a restful break! :) It's always a pleasure working with you all.

@SaravananSathyanandhaQC SaravananSathyanandhaQC deleted the quarto-1.4 branch March 14, 2024 15:25
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