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 GNU gettext #90

Merged
merged 6 commits into from
Jan 16, 2018
Merged

Add GNU gettext #90

merged 6 commits into from
Jan 16, 2018

Conversation

codingjoe
Copy link
Contributor

gettext is commonly used in various frameworks and
programming languages to provide support for i18n. Such as

  • Python/Django
  • Ruby/translation
  • Python/flask-babel
  • ... may more.

I does not make sense to have it available in side the runtime dyno, but during build time, it is helpful to have the package available to compile gettext po files (to mo).

There are already some buildpacks doing that, but since this is a GNU tool, it makes sense to have it in the main image.

[gettext][gettext] is commonly used in various frameworks and
programming languages to provide support for i18n. Such as
* Python/Django
* Ruby/translation
* Python/flask-babel
* ... may more.

I does not make sense to have it available in side the runtime dyno, but during build time, it is helpful to have the package available to compile gettext `po` files (to `mo`).

There are already some buildpacks doing that, but since this is a GNU tool, it makes sense to have it in the main image.

[gettext]: https://www.gnu.org/software/gettext/
@codingjoe codingjoe requested a review from a team January 3, 2018 11:03
@codingjoe
Copy link
Contributor Author

@tt what needs to be done to get this thing on the road? Anything I can do?

@@ -9,6 +9,7 @@ apt-get install -y --force-yes \
autoconf \
bison \
build-essential \
gettext \
Copy link
Contributor

@ojacobson ojacobson Jan 10, 2018

Choose a reason for hiding this comment

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

Hi, and thanks for the contribution!

One question. Putting the actual gettext package here will install the headers and the required .so files in the heroku-16-build image, but not in the heroku-16 image. The library will be available at build-time only. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, one should compile gettext files only during deploy not during runtime.

@dmathieu
Copy link

@codingjoe could you give us the size of the package when installed?

@codingjoe
Copy link
Contributor Author

@codingjoe could you give us the size of the package when installed?
@dmathieu what's the recommended why to measure that? Only package or including dependencies?

@edmorley
Copy link
Member

edmorley commented Jan 11, 2018

The Travis script outputs the image sizes to the log to help with decisions like these :-)

master:

  • heroku/heroku:16 461MB
  • heroku/heroku:16-build 852MB

HEAD~2 (this PR's parent until it's rebased) which doesn't have #89:

  • heroku/heroku:16 460MB
  • heroku/heroku:16-build 851MB

This PR:

  • heroku/heroku:16 461MB
  • heroku/heroku:16-build 858MB

@codingjoe
Copy link
Contributor Author

6MB than, thanks @ojacobson
How often/When do you release new images?

@dmathieu
Copy link

@codingjoe we release stack images when required. It just happens that we're releasing one today (without your patch).
We could release again next week though. I'd like to get some else from @heroku/build to approve the PR though.

@dmathieu
Copy link

ping @dzuelke too.
WDYT? Especially about having it only in -dev?

@codingjoe
Copy link
Contributor Author

@codingjoe we release stack images when required. It just happens that we're releasing one today (without your patch).
We could release again next week though. I'd like to get some else from @heroku/build to approve the PR though.

sounds cool, always a pleasure to work with you guys 🥇

@dzuelke
Copy link
Contributor

dzuelke commented Jan 15, 2018

Yeah the dev-only part is fine @dmathieu. Really all that's needed is msgfmt but we can only install the entire package. I suppose many people still produce .mo files locally and check them into version control, but thinking about it, it actually makes sense to do so at build time.

@codingjoe
Copy link
Contributor Author

codingjoe commented Jan 15, 2018

I suppose many people still produce .mo files locally and check them into version control, but thinking about it, it actually makes sense to do so at build time.

My point exactly 👍

@dmathieu dmathieu merged commit 8d15ddd into heroku:master Jan 16, 2018
@codingjoe codingjoe deleted the patch-1 branch January 16, 2018 10:30
@codingjoe
Copy link
Contributor Author

Awesome, docker works. How long does it take until the new image is available during the regular buildpack builds?

@dmathieu
Copy link

I'll let you know here once I've finished updating the stack image.

@dmathieu
Copy link

@codingjoe
Copy link
Contributor Author

Nice, I do get a msgfmt: Command not found but I guess I just need to wait a bit.

@dmathieu
Copy link

Yes, not all instances have finished updating yet. That'll be done in the next couple of hours.

@codingjoe
Copy link
Contributor Author

I still get make: msgfmt: Command not found even after two days. I call msgfmt from post_compile inside the Python buildpack :/

@dmathieu
Copy link

@codingjoe we're aware of the issue which is not related to your patch. We're working on it and will let you know once the stack image is shipped again.

@dmathieu
Copy link

@codingjoe sorry for the delay here. gettext should be available now.

@codingjoe
Copy link
Contributor Author

🎉 I can confirm it works 👍

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.

6 participants