-
Notifications
You must be signed in to change notification settings - Fork 41
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
Require httpclientjava instead of httpclient5 #206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good but I haven't tested it at all. I am pretty sure that the only way for this to be thoroughly tested is via I-build though.
Yes I'm also not sure how P2 will behave here... as it is only a prerequisite and not a (strict) requirement it might just install the new connector additionally? I think if the test suceed, we can merge and must see what happens... |
AFAIK testing p2 from a runtime Eclipse is not really possible so +1 for merging to see it used in an I-build (if the validation build is green). |
Seems it is not that easy... have to investigate further, but if someone else likes to take a look help would be welcome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to install a feature (EMFs SDK feature) in a self-hosted runtime where software installation is enabled. So it appears to work well for me.
395e139
to
0c1a603
Compare
0c1a603
to
00c6785
Compare
Time to merge this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for merging to see this in a running IDE. If we face issues we still can revert
I cannot assess the change, but if it is risky then we should it better sooner than later. |
It needs someone to sign up to keep an eye on it and it has to go for M1 if at all. I can land a hand but can't be the driving force here. So if someone feels like it today is the day or it would miss yet another release. |
I have rebased the change now, but I won't have time too look into the testfailures or give dedicated support for it. |
I can watch for issues, and offer to revert it ASAP if needed :-) |
It looks like all the builds so far are aborting: https://ci.eclipse.org/equinox/job/p2/job/PR-206/
Without timestamps in the log, one can't really tell which parts are taking so long, but each seems to abort during the p2 tests... Normal builds appear to take about 20 minutes: https://ci.eclipse.org/equinox/job/p2/job/master Similarly the p2 action build is taking very long: While a normal such build should take about 15 minutes: Something is either very slow or non-terminating... |
Yes I assume some test is waiting for "something" to happen and then blocks forever, but I haven't had time yet to investigate this. |
I would be very happey if this one lands in now. @laeubi do you plan to do so? |
I have rebased the change lets see what happens but last time the tests started to hang infinite... |
One of the test fail here:
The URl in question returns |
Sounds like something to be improved in ecf httpclient . |
I have created so it should work with an english locale, when execute the build I get some more errors that seem to be related to locales (but not to the transport) that is a bit unfortunate as it seems that
|
I fear we need a new ECF release first before we can proceed here but, It would be good if someone else could also execute the build locally (a simple |
Alright we have no longer a hang of the test but one test-failure that looks a bit strange: https://ci.eclipse.org/equinox/job/p2/job/PR-206/9/testReport/junit/org.eclipse.equinox.p2.tests.artifact.repository/TransferTest/testGZFileAreNotUnzipped/ |
This does not appear to me to have anything to do with ECF or the javahttpclient provider.
It seems strange to me that Bundle.getSymbolicName() would ever fail in the Eclipse testing environment but perhaps it's possible under some conditions? |
the bundle itself is null so maybe something has changed here where a specific bundle name is assumed, I'll take a look... I just wanted to point out that now tests are all running (even though not passing) that before where blocked forever so that is now fixed 👍 |
The problem is this line: It seems only a debugline so I'll adjust it, beside that the test seem to reference packed artifacts so probably should be removed at all @akurtakov ? |
3a699f8
to
6015fd0
Compare
The test referes to pack.gz file but this seems to be because this was the first gziped file found convenient. It could actually be any gzipped file. |
@vogella @merks @akurtakov the build now succeeds, I'll leave it up to you to merge this if desired. |
In the current shape there are high chances to fail the build as I'm not sure that all the products will find content from the extra p2 repo added here. |
In the meantime, couldn't we update the .target to use the snapshots of ECF and then merge this patch without the extra repo and keep a ticket open to ensure we update to ECF release ASAP ? |
If the snapshot is here to stay till a release - yes. |
Looks like the https://download.eclipse.org/rt/ecf/snapshot/site.p2/ contains snapshots which are more than 1 year old, so let's just use it in .target until there is a release ( @scottslewis the sooner the better ;) ). |
See eclipse-platform/eclipse.platform.releng.aggregator#1372 about updating the .target. |
@mickaelistria thanks! |
So we can start consuming the new httpclientjava and getting rid of httpclient5. The snapshot repo must be replaced by release one as soon as ECF provides a release, before Eclipse 4.30.M3. See eclipse-equinox/p2#206
So we can start consuming the new httpclientjava and getting rid of httpclient5. The snapshot repo must be replaced by release one as soon as ECF provides a release, before Eclipse 4.30.M3. See eclipse-equinox/p2#206
The ECF snapshot was added to the .target. Can you please amend the commit to remove the repository entry? |
6015fd0
to
d5b08e5
Compare
Done. |
Build fails with
In the .target with eclipse-platform/eclipse.platform.releng.aggregator#1372 , I've used |
d5b08e5
to
9003489
Compare
Merged. Thanks a lot! |
Would be nice to trigger an I Build now to verify if anything is broken. |
OK, I'm triggering I-Build. |
@mickaelistria if you like, I'll add this cleanup to my TODO list. I've already done some investigation previously for the impact on Oomph which is also directly using ECF. Here too the switch is mostly painless... |
OK, I happily leave it the continuation of the cleanup to you @merks ! |
I would prefer to wait one week for a full release of ECF, as a full release requires some extra releng that I don't have time for this week. Also, IMO it would be nice to have one platform integration release for a 'smoke test' of javahttpclient provider (and perhaps more difficult removing the httpclient5 provider and all org.apache.* deps) before I do a release as I don't see doing weekly releases of ECF as feasible for me right now. It seems that maybe you've already used the latest at https://download.eclipse.org/rt/ecf/snapshot/site.p2/, which is: https://download.eclipse.org/rt/ecf/snapshot/site.p2/3.14.40.v20230905-1933/ |
Fix #205