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

Websphere adaptions in forPackage and forResource #85

Closed
wants to merge 1 commit into from

Conversation

Xnyle
Copy link

@Xnyle Xnyle commented Jul 20, 2015

This change is the bare minimum needed so that this library is usable on Websphere.

Hope this minimal change gets accepted this time.

Websphere is not able to find resources if you don't add / to it.
This is especially a problem when using forPackage as the Websphere Classloader will never find anything!
In case of forResource you might find something but it might be incomplete!

forPackage
forResource

Websphere is not able to find resources if you don't add / to it.
This is especially a problem when using forPackage as the Websphere Classloader will never find anything
In case of forResource you might find something but it might be incomplete
@lorenzobenvenuti
Copy link

+1. I had to apply this patch in order to deploy my app on Websphere.

@ronmamo
Copy link
Owner

ronmamo commented Aug 10, 2015

I hate websphere

@martin-g
Copy link

Me too!
On Aug 10, 2015 5:36 PM, "ronmamo" notifications@github.com wrote:

I hate websphere


Reply to this email directly or view it on GitHub
#85 (comment).

@Xnyle
Copy link
Author

Xnyle commented Aug 10, 2015

Very mature comments. I think we can all agree that WAS is one of the worst servers out there. But what does that assessment help? Either I am able to used reflections on the server the customer had chosen and paid millions for to IBM or I'm not. If not I have to maintain my own fork and others who have to use WAS will run into the same trap...

@martin-g
Copy link

My reasoning is: if you/your client pays for WAS then you/your client should file a bug at WAS reporting the problem and demand a fix in WAS.
Otherwise every library (like Reflections) should add such noise in its code to deal with yet another non sense in WAS (or any other unfriendly product).
Until you (the paying customers) start asking IBM to fix its sh*t, WAS won't be improved and we (the OSS developers) will suffer (by having to apply such kind of code to our libraries).
Point me to your bug report at WAS issue tracker and I'll give my +1 for your patch.

@Xnyle
Copy link
Author

Xnyle commented Aug 11, 2015

Yet reality looks different. My chances convincing our customer to file a bug report are nil even if I spend hour's explaining and convincing 10 different ppl who all have (or think they have) a saying in "organizing" this kind of stuff on their side. So i won't do this as I'm bound to limited resources myself.

Besides, even if IBM by some miracle will fix this, they will probably never bring out another patch for the current server version which our customer will probably use until 2050...

So in principle you are right, but the approach just isn't pragmatic. I guess that's the reason other projects on Apache from which I basically stole the PR patched their libraries as well.

@ronmamo
Copy link
Owner

ronmamo commented Aug 13, 2015

@Xnyle, regardless of my prev. comment (had to say it, can't control it ;),
We can easily normalise resourceName in forPackage/forResource to end with '/'.
That is more elegant, would work well for other environments as well, and save us from specifically mentioning that app server (or another)
WDYT?

@Xnyle
Copy link
Author

Xnyle commented Aug 13, 2015

But what if I only want to find a certain file in forResource? I don't know if always adding a / causes other problems on other servers in that case .

Even in forPackage I'm not sure if some other implementations maybe don't accept a trailing /

I try to only change the behaviour on WAS and also add some warn/debug logs in that case, so I won't break anything on other implementations Additionally I give users a chance to detect that there is some magic happening if they look at the logs.

Might blow up the code a bit but OTOH it doesn't break things for other uses cases.

@ronmamo ronmamo closed this Feb 24, 2016
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.

None yet

4 participants