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

quarto: 1.5.57 -> 1.6.30 #349683

Merged
merged 1 commit into from
Nov 3, 2024
Merged

quarto: 1.5.57 -> 1.6.30 #349683

merged 1 commit into from
Nov 3, 2024

Conversation

detroyejr
Copy link
Contributor

The deno package now points to Deno v2 as of last week (#347484). However, there's currently a compatibility issue with quarto that's causing some problems so I've pinned to the previous version.

ERROR: TypeError: Attempted to load JSON module without specifying "type": "json" attribute in the import statement.

Fixes #349444.

CC maintainers @minijackson @MrTarantoga

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@natsukium
Copy link
Member

deno_1 is no longer maintained and has been deleted. See #347974

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

@detroyejr detroyejr force-pushed the quarto-deno-fix branch 2 times, most recently from 0ce43f6 to 9c5c005 Compare October 23, 2024 14:01
@detroyejr
Copy link
Contributor Author

Marked with the following warning:

Known issues:
    - Quarto requires deno version 1.46.3 which is EOL. See https://github.com/NixOS/nixpkgs/issues/349444#issuecomment-2431231202 for more details.

@detroyejr detroyejr marked this pull request as ready for review October 25, 2024 13:44
Copy link
Member

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

Hello.

Thanks for trying to resolve this issue. Apologies for not catching this earlier via the passthru tests when bumping deno.

I won't be helping maintain this separate deno expression. Until the quarto devs are more positive and friendly towards downstream consumer repos I'd appreciate not being CCed in issues/PRs for this particular expression.

If @emilazy is happy with the combination of using knownVulnerabilities + hiding deno 1 under this folder then I'm happy.
Otherwise our only option might be to drop quarto for the time-being.

I'd also recommend getting approval from the maintainers for this package before merging :)

I made a post on the quarto discussion giving some background and disputing some of the statements. Hopefully it'll do more good than bad 😅 the goal was to put things into perspective a bit.
I'll be focusing on what I can fix for next time at the very least.

Cheers

pkgs/development/libraries/quarto/deno.nix Outdated Show resolved Hide resolved
@detroyejr
Copy link
Contributor Author

detroyejr commented Oct 25, 2024

Hey, thanks for the feedback. I've updated the maintainers list.

The command being used to test the binary (quarto check) does return the same error but nixpkgs-review gave all green so it's not really your fault there. I'll see if this can be improved so that you/the nixpkgs quarto maintainers have better information when this syncs up with the the latest version again.

Both team have different priorities and I think that's mostly fine. The javascript ecosystem moves so fast, I'm not that surprised if teams have trouble keeping up with changes. The PR here, if accepted, seems like a good compromise and I think addresses some of their teams concerns as well. Hopefully Deno's plans to offer LTS in 2.1 helps with this situation too.

One more CC to the maintainers @minijackson @MrTarantoga for good measure. Thanks all.

Edit: Ah, okay nixpkgs-review doesn't run tests, but this might be added in the future.

@tjni
Copy link
Contributor

tjni commented Oct 26, 2024

Do things work if bumping up to the pre-release version? I tried 1.6.30 locally with the following changes, and quarto check passes (on macOS):

diff --git a/pkgs/development/libraries/quarto/default.nix b/pkgs/development/libraries/quarto/default.nix
index a3a633af6c94..b9b9171e217f 100644
--- a/pkgs/development/libraries/quarto/default.nix
+++ b/pkgs/development/libraries/quarto/default.nix
@@ -13,28 +13,22 @@
 , runCommand
 , python3
 , quarto
-, extraPythonPackages ? ps: with ps; []
+, extraPythonPackages ? ps: []
 , sysctl
 }:
 
 stdenv.mkDerivation (final: {
   pname = "quarto";
-  version = "1.5.57";
+  version = "1.6.30";
   src = fetchurl {
     url = "https://github.com/quarto-dev/quarto-cli/releases/download/v${final.version}/quarto-${final.version}-linux-amd64.tar.gz";
-    sha256 = "sha256-ZBjv/Z98il8EMZe88fMKSi1YjeOZ8jEh7OxYDKUTMpY=";
+    hash = "sha256-2gzpQbaFLUox4EMo8RO3bwVjhsm239w5hv4Z0UuS1Qs=";
   };
 
   nativeBuildInputs = [
     makeWrapper
   ];
 
-  postPatch = ''
-    # Compat for Deno >=1.26
-    substituteInPlace bin/quarto.js \
-      --replace-fail ']))?.trim();' ']))?.trim().split(" ")[0];'
-  '';
-
   dontStrip = true;
 
   preFixup = ''

I haven't tried anything else yet (I stumbled on this while newly trying to use quarto with molten-nvim, so I have no experience with this package before).

@detroyejr
Copy link
Contributor Author

Thanks for chiming in. That does indeed work and should have been the first thing I checked. I'll will check pre-releases in the future.

I'm not seeing anything wrong with the basic functionality and the project I use this for also doesn't have any issues with it, so I think we can move forward with this instead.

@detroyejr detroyejr changed the title quarto: use deno version 1 quarto: 1.5.57 -> 1.6.30 Oct 26, 2024
@b-rodrigues
Copy link
Contributor

b-rodrigues commented Oct 29, 2024

thank you for this @detroyejr and @tjni I was just bit by the bug #349444 and am glad there's a fix! However I'm not sure I understand why this works, latest quarto pre-realese doesn't rely on Deno 2 either?

@detroyejr
Copy link
Contributor Author

This is my best guess at the moment for why this works. When nixpkgs jumped from 1 to 2, I assumed this broke because of the major version change. However, quarto now uses a newer minor version of deno 1 as of 3 weeks ago and that's probably the reason this pre-release works.

In other words, 1.41.0 -> 1.46.3 is compatible (the last version of deno v1 nixpkgs supported) and 1.46.3 - > 2 works well enough based on this PR, but 1.41.0 -> 2 does not. If quarto was built from source we'd have other deno issues to solve as noted by the quarto team, but there's enough compatibility between the two versions that this works for now.

@b-rodrigues
Copy link
Contributor

b-rodrigues commented Oct 29, 2024

I see this makes sense

btw, and sorry for the off topic, but there's a matrix channel dedicated to the R language where we coordinate, review each other's PRs and so on

https://matrix.to/#/#r:nixos.org

would be nice if you all could join

@tjni
Copy link
Contributor

tjni commented Nov 1, 2024

@emilazy would you mind taking another look at this one? we think we found a way to move forward on Deno 2.

Copy link
Member

@minijackson minijackson left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 349683 run on x86_64-linux 1

6 packages built:
  • quarto
  • quartoMinimal
  • rstudio
  • rstudio-server
  • rstudioServerWrapper
  • rstudioWrapper

1.6.30 is still a pre-release, but I think it's acceptable since the package is currently broken, due to the Deno 2 issue.

From the release schedule point of view, I think the Quarto breaking change is also fine, for the same reason.

Sorry if I have been slow to respond. I don't have many occasions to use Quarto anymore, so if someone wants to replace me as maintainer, feel free to step up!

@emilazy emilazy dismissed their stale review November 2, 2024 20:51

No longer relevant

@emilazy
Copy link
Member

emilazy commented Nov 2, 2024

I’m a little hesitant about this one. I’m definitely glad that it no longer involves reintroducing Deno 1! But it seems like Quarto upstream may be unwilling to support any package that doesn’t pin their entire exact dependency tree, which is obviously ot viable for us, and I’m not sure that we have any easy way of marking that users should go to us for support first. If they’re worried about support burden, then shipping a preview release might make that situation worse. From the discussions, it seems likely that Quarto not working with the version of Deno we package will continue to happen in future, so I’m a bit worried about the sustainability of this package. On the other hand, I don’t think that upstreams should have an absolute veto on what we package, and it’s unclear to me how much the feelings on our package expressed reflect that of individual Quarto contributors vs. project‐wise consensus.

This PR is an improvement over the status quo, since our current package is completely non‐functional, so I’m removing my blocking review, but I’d feel more confident if someone else were to approve this one. I’ll link it in the review requests chat.

@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Nov 2, 2024
Copy link
Contributor

@tjni tjni left a comment

Choose a reason for hiding this comment

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

I think we should keep doing what's convenient for us, just as upstream is doing what is convenient for them, with the understanding that they don't have resources to help us. Perhaps we'll find out over time that they're right and that maintenance becomes a huge burden, in which case we can change course.

In the meantime, this change looks good to me too.

@tjni tjni merged commit 495188f into NixOS:master Nov 3, 2024
35 checks passed
@b-rodrigues
Copy link
Contributor

From the discussions, it seems likely that Quarto not working with the version of Deno we package will continue to happen in future

I guess shipping a patched ubuntu binary would be a solution ?

@shriv
Copy link

shriv commented Nov 4, 2024

Thanks for sorting out this fix @detroyejr! 🙏 🙏
I too get the issue in aarch darwin. However, I built a new nix env 16h after this PR was merged. Do you know how long till I can get quarto 1.6.30 instead of 1.5.57?

@detroyejr
Copy link
Contributor Author

@shriv assuming you're using nixpkgs unstable it usually only takes a few days. You can see where it's currently available here.

@shriv
Copy link

shriv commented Nov 4, 2024

Thanks @detroyejr! I am indeed using nixpkgs unstable. I will wait a little longer before rebuilding the env 👍

@carschandler
Copy link

@shriv https://nixpk.gs/pr-tracker.html?pr=349683

@detroyejr detroyejr deleted the quarto-deno-fix branch November 8, 2024 19:11
@gkapfham
Copy link

gkapfham commented Nov 8, 2024

Hi! I know that this has been merged and the branched connected with this PR was closed about 2 hours ago. However, I only had time a few minutes ago to test quarto at version 1.6.30 and it does not work for me.

Here is what I see:

❯ quarto preview
Preparing to preview

ERROR: TypeError: Deno.copy is not a function

Stack trace:
    at SAXParser.parse (file:///nix/store/1p9xhdnmz13nzalf88i53q6z5l45p070-quarto-1.6.30/bin/quarto.js:95972:24)
    at readSitemap (file:///nix/store/1p9xhdnmz13nzalf88i53q6z5l45p070-quarto-1.6.30/bin/quarto.js:96083:18)
    at eventLoopTick (ext:core/01_core.js:175:7)
    at async updateSitemap (file:///nix/store/1p9xhdnmz13nzalf88i53q6z5l45p070-quarto-1.6.30/bin/quarto.js:96038:28)
    at async websitePostRender (file:///nix/store/1p9xhdnmz13nzalf88i53q6z5l45p070-quarto-1.6.30/bin/quarto.js:98864:5)
    at async Object.postRender (file:///nix/store/1p9xhdnmz13nzalf88i53q6z5l45p070-quarto-1.6.30/bin/quarto.js:98830:9)
    at async renderProject (file:///nix/store/1p9xhdnmz13nzalf88i53q6z5l45p070-quarto-1.6.30/bin/quarto.js:85755:13)
    at async serveProject (file:///nix/store/1p9xhdnmz13nzalf88i53q6z5l45p070-quarto-1.6.30/bin/quarto.js:107232:24)
    at async Command.actionHandler (file:///nix/store/1p9xhdnmz13nzalf88i53q6z5l45p070-quarto-1.6.30/bin/quarto.js:107927:9)

❯ quarto --version
1.6.30

This is the site on which I am trying to run quarto: https://github.com/TeamDevDev/www.developerdevelopment.com

This is my NixOS configuration: https://github.com/gkapfham/nixos

The migration guide for Deno suggests that copy should be used from the standard library instead of using the function that is reported as missing. Reference: https://docs.deno.com/runtime/reference/migration_guide/

Is there any other way to make quarto work correctly in NixOS unstable? Is there any other diagnostic information that I need to provide?

@detroyejr
Copy link
Contributor Author

detroyejr commented Nov 8, 2024

@gkapfham That's unfortunate. I can get the site to render under certain conditions but it's not consistent. Would you mind opening up an issue for this and we can discuss there?

@gkapfham
Copy link

gkapfham commented Nov 8, 2024

Hi @detroyejr, yes, I can probably open up an issue in the next three days. I regret that I cannot immediately do so due to the fact that I have to complete other tasks first! It is super-interesting to learn that you can get the site to render under specific circumstances; I look forward to learning more details once I raise the issue.

I've never contributed to this repository beyond reading issues and tracking the status of PRs: from your perspective, anything I need to first know? (If not, don't feel obligated to respond to this comment as I'll do my best to raise a new issue soon so that we can discuss a suitable solution for ensuring that a recent version of Quarto works with Nix and NixOS).

@detroyejr
Copy link
Contributor Author

Use the bug report issue template if that's helpful, but don't feel like you have to fill it all out. Your previous comment is good enough. Ping me and drop a link to this PR in the body of the issue so that there's a trail, but we're not involving everyone here in that discussion if they're not interested. I think that's it!

@emilazy
Copy link
Member

emilazy commented Nov 8, 2024

I really think we have to consider marking this broken for 24.11, since it’s clearly not just a build‐time bundling problem but runtime used of removed APIs too. I am sceptical that there will be any way to get this codebase properly running on Deno 2 that doesn’t involve doing the full port that upstream is currently considering embarking upon.

@detroyejr
Copy link
Contributor Author

Yeah, perhaps. FWIW the side of quarto that I use to render Word/PDF documents has been working fine. So it's not completely broken. Let me know if you want me to open up a PR.

@gkapfham regardless of whether or not this is marked as broken, we can still discuss a fix for your use-case.

@tjni
Copy link
Contributor

tjni commented Nov 9, 2024

Can we try #354672 to fix it until this can be pushed back upstream? If there is more incompatibility noticed, I can fix this patch too.

@shriv
Copy link

shriv commented Nov 13, 2024

@detroyejr @b-rodrigues @tjni @emilazy
I can get 1.6.30 to work well with pdf output where figures are generated with Python or an HTML with figures generated R. However, markdown figures are not scaling correctly. I created an issue on quarto-cli but I'm not sure where the problem is coming from! I've created a reprex repo for what I'm seeing.

I've got a temporary solution that renders the pdf correctly with quarto 1.4.554, TexliveSmall and texlivePackages.latexmk from nixpkgs 24-05 instead of unstable but I'd really like to be able to use recent versions of quarto.

Let me know if I can run any tests / checks to figure out what is going on! :-)

@tjni
Copy link
Contributor

tjni commented Nov 14, 2024

The repro was very useful.

This is an issue on our side: changes in quarto's 1.6 branch require at least pandoc 3.2.1 to fix image scaling, but we only have 3.1.11.1. I also verified that the images render correctly using pandoc 3.4 (what quarto 1.6 pins) and pandoc 3.5 (the latest released version).

This update was requested in #348028, but I don't know if there is a way for us to get it quicker. @maralorn do you have any suggestions?

@emilazy
Copy link
Member

emilazy commented Nov 14, 2024

I don’t think it’s possible to bump Pandoc beyond the Stackage LTS version in 24.11. It would rebuild many packages and likely break things. I am really not sure that we can sustain this package under the constraints we have.

@maralorn
Copy link
Member

Huh, this sucks. If there is sufficient demand we can certainly try unpinning pandoc. It’s a bit of work and we will have to observe the fallout, but I think it’s feasible. However branch-off is today and we won’t get in any more Haskell merges. So I don’t know how to do this for 24.11.

@tjni
Copy link
Contributor

tjni commented Nov 14, 2024

No problem! I can package a pandoc binary in the meantime for this package.

@emilazy
Copy link
Member

emilazy commented Nov 14, 2024

I am concerned that we are indeed starting down the route of building out an entire parallel ecosystem for one package. If we have to have multiple Pandoc versions then I would prefer to have multiple versions of the source package if possible.

@maralorn
Copy link
Member

We likely don’t need multiple versions at all and if we did we could do it from source.

@emilazy
Copy link
Member

emilazy commented Nov 14, 2024

I think part of the problem is that upstream also don’t care to support any version newer than their exact pins. So pinning Pandoc for the entire distro based on one binary package we ship seems unreasonable, though maybe we could get away with just shipping the latest more than we could the current policy. (Still, it doesn’t seem unlikely that it’ll turn out that another mismatching Quarto dependency version will become the next problem.)

@emilazy
Copy link
Member

emilazy commented Nov 19, 2024

What’s the status here? I’m worried we’re on track to ship a broken (but not marked as such) quarto in 24.11.

@carschandler
Copy link

I'm just a quarto user but I do agree with @emilazy's sentiment. It's better to have users know that the package is not 100% functional so that they can consider alternatives on the front-end rather than have something partially working conk out on them in a critical moment (read: procrastinating a homework assignment/paper) only to come find all the evidence of it here.

@tjni
Copy link
Contributor

tjni commented Nov 19, 2024

My thoughts on this:

  1. Upstream seems to work on updating to newer versions of dependencies but only commits to maintaining pinned versions in their released versions. That's why I felt that the pre-release version was the best equilibrium to be in: we have a better chance that quarto is compatible with the versions we maintain, and we can (with tact), report or contribute fixes upstream to deal with newer versions when necessary. We still haven't had enough time to conclude whether this policy works better or not with the quarto package.

  2. What should we do about pandoc being on an older version? I view it similarly to other situations when we have some package that is depended upon and is hard to update; at some point, it will be updated, and in the meantime if someone cares enough to maintain it, we package the new version as its own derivation.

  3. On packaging a newer version of pandoc from source or as a binary: in theory I also prefer source. In practical terms, I'm not a Haskell user, but I can try to start with the pandoc-cli-3_4 Haskell package and find a consistent dependency set which will build, with the blessing of Haskell maintainers. Otherwise, I don't mind packaging a binary and maintaining it in the meantime.

  4. I have not had the time to create this package yet. So whether to mark this broken or not, I don't have a strong opinion. It sounds like this pandoc incompatibility is serious enough that it's bitten folks, so it feels reasonable to mark it broken with a message on what needs to happen to unbreak it.

@emilazy
Copy link
Member

emilazy commented Nov 19, 2024

I’m okay trying packaging a newer Pandoc for 24.11 if we think it will solve the outstanding known issues; just worried about being due to release in ~a week’s time with this package being subtly broken. I would recommend joining the Nix Haskell room on Matrix if you need help packaging it. I’m not sure how best to do multiple versions given the automatically‐generated nature of the Haskell package set.

@detroyejr detroyejr mentioned this pull request Nov 20, 2024
13 tasks
@detroyejr
Copy link
Contributor Author

The above PR builds a working quarto with pandoc 3.5. We may want some input from someone who actually knows Haskell. Planning to do some more testing and clean up before marking it as ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

quarto cannot render