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

Follow P2 contract of cached file's extension (#2938) #2945

Merged
merged 4 commits into from
Oct 22, 2023

Conversation

basilevs
Copy link
Contributor

Fixes #2938

  • Current implementation of CacheManager injected from Tycho into P2 names cached downloads internally based on their URLs
  • When HTTP server performs a redirect, URL and cache file name change to reflect that.
  • Some servers (Github) redirect to URLs that do not follow any particular convention on path or file names.
  • Tycho caches such files with invalid names.
  • The invalid name is then returned from cache layer and confuses P2.
  • P2 relies on correct file extension to parse cached files.
  • Files with invalid names are considered to be XML
  • P2 fails to parse files with invalid names with an error "Content is not allowed in prolog" even if the file content is a valid JAR.

This change ensures that CacheManager injected into P2 adheres to the contract and returns cached files with extensions matching the (original, before redirect) request.

Integration test infrastructure touch-ups:

URLs returned from HttpServer.getAccessedUrls() are now stripped of context prefix. No callers have used these values until now. Fix concurrency bug in test utility class HttpServer request logging.

P2 relies on correct file extensions to parse cached files.

Fixes eclipse-tycho#2938

See P2 bug:
eclipse-equinox/p2#355

Integration test infrastructure touch-ups:

Fix HttpServer concurrency bug.
URLs returned from HttpServer.getAccessedUrls() are now stripped of
context prefix. No callers have used these values until now.
Fix concurrency bug in test utility class HttpServer request logging.
}

public String getUrl(String contextName) {
return "http://localhost:" + port + "/" + contextName;
}

public List<String> getAccessedUrls(String contextName) {
return contextName2servletsMap.get(contextName).getAccessedURIs();
synchronized (contextName2accessedUrls) {
return List.copyOf(contextName2accessedUrls.get(contextName));
Copy link
Member

Choose a reason for hiding this comment

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

This seem to introduce test failures now as some test afterwards modify the collection see:
https://ci.eclipse.org/tycho/job/tycho-github/job/PR-2945/1/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bit of a struggle, as it is not trivial to run integration tests locally for Tycho 5.0.x.
#2946
#2947

OfflineModeTest fails with UnsupportedOperationException as HttpServer
no longer allows direct access to access logs. Change OfflineModeTest to
dedicated and limited mutation API for access logs.
Improve variable locality.
Make error log assertion first, to facilitate investigation of
non-network failures.
@basilevs
Copy link
Contributor Author

org.eclipse.tycho.test.surefire.RunOrderTest.testRunOrder is unstable, I've failed to reproduce the failure locally.

MultiMap.get() may return null.
@laeubi
Copy link
Member

laeubi commented Oct 22, 2023

Test failures seem to be caused by eclipse infrastructure errors...

@basilevs
Copy link
Contributor Author

Test failures seem to be caused by eclipse infrastructure errors...

I'm not sure about that. Are those test supposed to access internet at all?

@laeubi
Copy link
Member

laeubi commented Oct 22, 2023

Test failures seem to be caused by eclipse infrastructure errors...

I'm not sure about that. Are those test supposed to access internet at all?

Yes the integrationtests access e.g. maven or eclipse updatesites. Lets see if restarting the build makes them pass.

@github-actions
Copy link

Test Results

   564 files  +  3     564 suites  +3   4h 41m 31s ⏱️ + 10m 47s
   368 tests +  4     362 ✔️ +  4    6 💤 ±0  0 ±0 
1 104 runs  +12  1 085 ✔️ +12  19 💤 ±0  0 ±0 

Results for commit 3da43cc. ± Comparison against base commit c1842b2.

@basilevs basilevs marked this pull request as ready for review October 22, 2023 14:36
@basilevs
Copy link
Contributor Author

basilevs commented Oct 22, 2023

Aggregator integration failure is unclear to me, but it also happens on other unrelated PRs. The PR is ready for review.

Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:5.0.0-SNAPSHOT:compile (default-compile) on project org.eclipse.pde.core: Compilation failure: Compilation failure: 
Error:  /home/runner/work/tycho/tycho/platform/eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/IUBundleContainer.java:[321] 
Error:  	IQueryable<IInstallableUnit> slice = slicer.slice(fUnits, new NullProgressMonitor());
Error:  	                                            ^^^^^
Error:  The method slice(Collection<IInstallableUnit>, IProgressMonitor) in the type Slicer is not applicable for the arguments (IInstallableUnit[], NullProgressMonitor)
Error:  /home/runner/work/tycho/tycho/platform/eclipse.pde/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/P2TargetUtils.java:[1364] 
Error:  	IQueryable<IInstallableUnit> slice = slicer.slice(units, subMonitor.split(50));
Error:  	                                            ^^^^^
Error:  The method slice(Collection<IInstallableUnit>, IProgressMonitor) in the type Slicer is not applicable for the arguments (IInstallableUnit[], SubMonitor)

@laeubi laeubi merged commit 8b64e14 into eclipse-tycho:master Oct 22, 2023
7 of 8 checks passed
@basilevs basilevs deleted the issue_2938_for_master branch October 22, 2023 14:53
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.

Content is not allowed in prolog
2 participants