search

conda-forge / quarto-feedstock

A conda-smithy repository for quarto.
BSD 3-Clause "New" or "Revised" License
4 stars 9 forks source link

Contribute "external installer" patch to upstream #27

Closed mfisher87 closed 7 months ago

mfisher87 commented 1 year ago

I thought this had already been done, or a TODO maybe created, but I'm having trouble finding it.

This patch:

https://github.com/MattF-NSIDC/quarto-feedstock/blob/95a3bd9494f63a8d1eafd18395be9437e31943d3/recipe/0001-add-external-installer-command-for-packagers-like-co.patch

Should be incorporated upstream, IMO.

Upstream issue: https://github.com/quarto-dev/quarto-cli/issues/6575

Once this is completed :point_up: we need to remove our patch and leverage the new installer/preparer.

FYI: @cderv

cderv commented 1 year ago

Yes I think we could do that.

@dragonstyle any reason that would prevent us to add this new internal command for quarto-bld ?

dragonstyle commented 1 year ago

This certainly could be added to quarto-bld though I'd like to suggest that we name it something like prepare-external, as I think what this is doing to closer to what our 'prepare' commands do (e.g. preparing the directory for building an installer).

cderv commented 1 year ago

I have open an issue to track on quarto-cli side. @mfisher87 do you want to do a PR ?

Otherwise, I can probably do one and ping you for review.

mfisher87 commented 1 year ago

Hey Christophe, I would love to but I think realistically, it would take me quite some time to get a PR ready. I don't get time at $DAY_JOB to do open source contributions unless I can tie them to a specific grant (looking for this to change, one way or another), and outside of work I'm committed to some group work on conda-lock and teaching Python for the next several weekends.

I would be happy to review if you wouldn't mind opening the PR!

EDIT: Updated the description of this ticket to remind us to remove the patch when the upstream change is done.

cderv commented 1 year ago

Sure - no problem, I'll do a PR in quarto for you to test building a new conda package with it.

cderv commented 1 year ago

@mfisher87 what is the email address associated to your github account so that I can add you as Co-author on the commit in Quarto ? no-reply address is fine -see https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors#required-co-author-information

Asking because I see no public email in your github profile. thanks !

Also if not done already, it would mean signing the contributor agreement I believe as for PR - links are in the doc here: https://quarto.org/about.html#pull-requests

For significant changes (e.g more than small bug fixes), ensure that you have signed the individual or corporate contributor agreement as appropriate. You can send the signed copy to jj@rstudio.com.

Thank you.

mfisher87 commented 1 year ago

Hey @cderv it is on my profile, but a bit buried. I'll make it more prominent :) mfisher87@gmail.com

mfisher87 commented 1 year ago

Woops, after a tiny bit of further thought about this, it's actually @msarahan who should be credited for this change.

cderv commented 1 year ago

I put you both. You did the last modification with TinyTeX patch I believe.

cderv commented 1 year ago

So https://github.com/quarto-dev/quarto-cli/pull/6730 can be tested now.

It requires 3 changes in there:

@mfisher87 something you can do to close the issue on side once after we merge https://github.com/quarto-dev/quarto-cli/pull/6730 ?

mfisher87 commented 1 year ago

Yes, happy to take the task of cleaning up the patches and build scripts and re-testing. May not be able to get to it until the weekend, though!

cderv commented 9 months ago

This was in fact opened since a long time at

And it was done recently at https://github.com/quarto-dev/quarto-cli/commit/0fea9ec2462db44e3e343b8eabe86950b90117fe

It just requires adjustment in the command name used.

mfisher87 commented 9 months ago

Woops. Let's let the linked PR autoclose this.

mfisher87 commented 7 months ago

Resolved in #36