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

get rid of Jammit dependency in production mode #262

Closed
wants to merge 1 commit into from

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Jun 28, 2012

In production, the only thing we need from Jammit is a set of helper methods.
Defining them lets us avoid all the pain of maintaining all the Jammit
dependencies.

ACKs
@iNecas
@lzap
@jlsherrill
@ehelms
@xsuchy

In production, the only thing we need from Jammit is a set of helper methods.
Defining them lets us avoid all the pain of maintaining all the Jammit
dependencies.
@lzap
Copy link
Contributor

lzap commented Jun 28, 2012

I am in, if that works! Great.

@jlsherrill
Copy link
Member

Are there really that many deps of jammit? I'm not a huge fan of including our own implementation of these 3 functions, but i guess if there are enough deps its worth it?

@ehelms
Copy link
Member

ehelms commented Jun 28, 2012

I might have missed a place to look, so could you supply a list of all the dependencies? Looking at Jammit on both rubygems and it's gemfile, I don't see any production level dependencies.

@iNecas
Copy link
Member Author

iNecas commented Jun 28, 2012

There is not problem with the amount of dependencies but yui-compressor bundling jar file into its gem, which is no good.

@jlsherrill
Copy link
Member

So i could see that causing a problem for Fedora inclusion, but if its a
build requires we would still need it in fedora in order for katello to be
in fedora, correct?

-Justin

On Thu, Jun 28, 2012 at 9:05 AM, Ivan Necas <
reply@reply.github.com

wrote:

There is not problem with the amount of dependencies but yui-compressor
bundling jar file into its gem, which is no good.


Reply to this email directly or view it on GitHub:
#262 (comment)

@xsuchy
Copy link
Contributor

xsuchy commented Jun 28, 2012

jammit brings in closure_compiler, yui_compresor. Thats 3 packages we do not need to support.

@jlsherrill
Copy link
Member

By support do you mean maintaining an rpm in our katello repo?

I'm still not sure what the purpose of this change is. Maintaining our own
versions of these three functions (a hack in my mind) just to get out of
having to package 3 rpms that are already packaged by us. Doesn't seem
worth it to me?

On Thu, Jun 28, 2012 at 9:20 AM, Miroslav Suchý <
reply@reply.github.com

wrote:

jammit brings in closure_compiler, yui_compresor. Thats 3 packages we do
not need to support.


Reply to this email directly or view it on GitHub:
#262 (comment)

@jlsherrill
Copy link
Member

We would also need to still package and ship it as part of System Engine.
So again I'm not sure what we're trying to accomplish here :)

On Thu, Jun 28, 2012 at 9:20 AM, Miroslav Suchý <
reply@reply.github.com

wrote:

jammit brings in closure_compiler, yui_compresor. Thats 3 packages we do
not need to support.


Reply to this email directly or view it on GitHub:
#262 (comment)

@iNecas
Copy link
Member Author

iNecas commented Jun 28, 2012

The less packages we need to install on the users computer the better. Especially in case when there is some bundled byte-code. And I believe there is a difference in supporting build and runtime dependencies.

But I agree that it solves only a little part of the whole problem we have with this.

Another thing it migth bring is being independent on one compressor. But that's probably a minor issue.

@ehelms
Copy link
Member

ehelms commented Jun 28, 2012

I would agree trying to be compressor independent is a minor thing. We only have to compress once for production, and the particular compressor we choose is just whatever preference we decided to go with.

@lzap
Copy link
Contributor

lzap commented Jul 2, 2012

@ehelms @jlsherrill - the point is Mirek is working on getting Koji working and this could reduce deps by three rubygems. If we ship it it does not mean we should never ever remove deps. Afters our support period, we can forget about security updates for them... 👍

@jlsherrill
Copy link
Member

Still not a fan of this as we are 'throwing away' rpms even though:

  1. We still have to package them for brew
  2. We still have to package them to get them into fedora
  3. They are already packaged

We are adding hacks in place of just keeping them. So if the three of you
want to be involved in

i. repacking/maintaing all the needed rpms of jammit for brew
ii. repacking/maintaining all the needed rpms of jammit for fedora when
the time comes

then feel free to keep and maintain the hacked in methods.

Again I don't see how this lets us 'avoid all the pain of maintaining
Jammit dependencies' because of i) & ii). If you are ok with signing up
for i) now & ii) (when the time comes) then I'll ack.

-Justin

On Mon, Jul 2, 2012 at 10:38 AM, Ivan Necas <
reply@reply.github.com

wrote:

In production, the only thing we need from Jammit is a set of helper
methods.
Defining them lets us avoid all the pain of maintaining all the Jammit
dependencies.

ACKs
@iNecas
@lzap
@jlsherrill
@ehelms
@xsuchy


Reply to this email directly or view it on GitHub:
#262

@lzap
Copy link
Contributor

lzap commented Jul 2, 2012

I agree with the opinion the patch seems to be "hacky". Considering to undo my ACK (uh how to do that :-)

@jlsherrill Is your avatar from the bar we have been to when I was in Raleigh? ;-D

@ehelms
Copy link
Member

ehelms commented Jul 2, 2012

I agree with @jlsherrill and can't justify to myself including a hack just to remove dependencies. Let's be good custodians of Open Source software. If a particular project we depend on is written in a way that makes life difficult or is wrong or buggy, we should fork it and makes the changes to the project itself. And, hope that we can get our changes accepted upstream to that project.

@knowncitizen
Copy link
Contributor

@iNecas Does this make it so you can still do any jammit related tasks including compression at build time? Does jammit image embedding get turned off, then?

@lzap
Copy link
Contributor

lzap commented Jul 2, 2012

That's actually good point - Ivan do you think upstream would accept this patch somehow? Not sure if this is feasible with bundler...

@iNecas
Copy link
Member Author

iNecas commented Jul 2, 2012

Ok guys, you're probably right. If the gems wouldn't bundle those jar files, it would be much better. Let's try to swim upstream then! Maybe we will be lucky.

Thanks all for comments and ideas! Closing for now. (If you ever wanted to get rid of this dependency again, you know where the code is:)

@iNecas iNecas closed this Jul 2, 2012
@iNecas
Copy link
Member Author

iNecas commented Jul 2, 2012

As @ehelms pointed out, newer version of jammit already got rid off the yui-compressor and other tools in production mode. So updating to the newer version will have the same effect as this pull request (and will be clearer way to do this).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants