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

Add needed packages to readme #18

Merged
merged 3 commits into from
Jun 20, 2022
Merged

Conversation

josephfrazier
Copy link
Contributor

Without these, I was getting errors about various .pc files being missing, see #5 (comment)

Without these, I was getting errors about various `.pc` files being missing, see Newlywords#5 (comment)
@brandoncc
Copy link
Contributor

Thanks for this PR. Do you use sharp? I'm wondering if your need for these packages is coming from the use of that library, rather than the buildpack itself.

I ask because the issue you were originally referencing was about using sharp as far as I understand.

Before we say everyone needs these packages, I'd like to make sure that is the case. I'm my own testing, I haven't seen any of the issues you were having.

Can you tell me when you where experiencing the errors? Was it during build? Or after you ran a certain command?

What type of application are you using the buildpack on? I use it in a ruby on rails application, so maybe different types of applications have different needs. If that is the case, I would like to clarify it on the readme before merging.

@josephfrazier
Copy link
Contributor Author

Thanks for the quick response! I do use sharp, and I think you're right in that I extrapolated from my use case. After I solved my problem in #5, I was probably too quick to assume that others would be experiencing the same problem. This change probably belongs in the sharp repo, so I'll close this and open a PR there. Sorry for the noise!

@brandoncc
Copy link
Contributor

No need to apologize for the noise. There seems to be a common enough use case that we should probably address it here too. I think we should add a section to the readme for sharp users which contains your changes. Would you be up for opening that new PR?

If you do get the changes into the sharp repo instead, we should at least include a link in the readme of this repo.

I really appreciate you sharing your solution so everyone can find it!

@brandoncc brandoncc reopened this Nov 28, 2021
@brandoncc
Copy link
Contributor

Edit: I said new PR, but then I reopened this one. You are welcome to make the changes here or in a new PR :-)

@josephfrazier
Copy link
Contributor Author

No need to apologize for the noise. There seems to be a common enough use case that we should probably address it here too. I think we should add a section to the readme for sharp users which contains your changes. Would you be up for opening that new PR?

If you do get the changes into the sharp repo instead, we should at least include a link in the readme of this repo.

I opened up a PR there, at lovell/sharp#2992, will wait to see how it goes before making more changes here.

I really appreciate you sharing your solution so everyone can find it!

Sure thing, and thanks for the buildpack in the first place!

@josephfrazier
Copy link
Contributor Author

lovell/sharp#2992 didn't catch on, so I'd be happy to help add the docs here. Do you have a particular format/location in mind?

@brandoncc
Copy link
Contributor

Hi @josephfrazier, I'm sorry I dropped the ball on this. I just merged a PR that does a great job updating the dependency information in the readme. If you would like to expand that documentation with a section about sharp, I'd be happy to merge. I think your PR from lovell/sharp was great, and you could just move that content over here and reference lovell/sharp in your comment. Of course, you should only need to reference the extra packages that you needed to add (just as this PR currently does).

Thank you for this contribution, I appreciate it.

@josephfrazier
Copy link
Contributor Author

No worries, I know how busy things can get! I just pushed an update that moves the extra packages into a section that mentions/links sharp, let me know what you think.

@brandoncc brandoncc merged commit e0a1b60 into Newlywords:master Jun 20, 2022
@brandoncc
Copy link
Contributor

This is great, thank you for spending time on this.

@josephfrazier josephfrazier deleted the patch-1 branch June 20, 2022 15:52
@josephfrazier
Copy link
Contributor Author

Thank you!

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.

2 participants