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

Fix org.apache.jasper.glassfish dependency in infocenter.product #688

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

merks
Copy link
Contributor

@merks merks commented Sep 15, 2023

@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

@iloveeclipse

Should we just merge this so we can kick off an I-Build?

@iloveeclipse
Copy link
Member

@iloveeclipse

Should we just merge this so we can kick off an I-Build?

Don't we need batch compiler in this product?

@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

@iloveeclipse
Should we just merge this so we can kick off an I-Build?

Don't we need batch compiler in this product?

Maybe? Probably? This product is plugin-based not feature-based so maybe that makes a difference?

It looks like this verification build will build the infocenter so we might as well wait for that the build here first...

@iloveeclipse
Copy link
Member

In doubt, I would propose to add batch compiler.

@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

I think it wouldn't hurt and we already realize that we need it (it's package export) in other places so I tend to agree.

Note that we have other "strange dependencies" that I didn't understand their purpose also related to this theme:

<requirement>
<type>eclipse-plugin</type>
<id>org.eclipse.jdt.core.compiler.batch</id>
<versionRange>0.0.0</versionRange>
</requirement>
<requirement>
<type>eclipse-plugin</type>
<id>org.mortbay.jasper.apache-jsp</id>
<versionRange>0.0.0</versionRange>
</requirement>
</extraRequirements>
</dependency-resolution>

@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

@iloveeclipse

FYI, it looks like the continuous-integration/jenkins/pr-head build got past building the infocenter-product:

09:03:56.831 [INFO] --- tycho-apitools:4.0.2:verify (verify) @ infocenter-product ---
09:03:56.833 [INFO] 
09:03:56.833 [INFO] ---------< eclipse.platform.ua:org.eclipse.ui.intro.universal >---------

Do you still think it's best to include the batch compiler immediately?

@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

Everything appears to have build, but there continue to be test failures that may well be because of missing things but those things might handled previous via the pom dependencies as mentioned here:

#688 (comment)

@iloveeclipse

Please advise at how you would like to proceed... I think/hope that with these changes the I-Build would work, although with test failures...

@iloveeclipse
Copy link
Member

Compile time doesn't need "jsp compiler" but at runtime product will miss the compiler package for sure.
Ed, please add batch compiler to the product, merge PR & start I Build.

@iloveeclipse
Copy link
Member

Note that we have other "strange dependencies" that I didn't understand their purpose also related to this theme:

No idea. I guess an attempt to make similar change like we do now.

@github-actions
Copy link
Contributor

Test Results

       42 files  ±0         42 suites  ±0   53m 29s ⏱️ + 2m 51s
  3 771 tests ±0    3 762 ✔️ ±0    3 💤 ±0    4 ±0  2 🔥 ±0 
11 316 runs  ±0  11 271 ✔️ ±0  27 💤 ±0  12 ±0  6 🔥 ±0 

For more details on these failures and errors, see this check.

Results for commit 4895bca. ± Comparison against base commit e5f0703.

@merks merks merged commit 13353c0 into eclipse-platform:master Sep 15, 2023
5 of 10 checks passed
@merks merks deleted the issue-aggr-1251-infocenter branch September 15, 2023 09:21
@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

@merks
Copy link
Contributor Author

merks commented Sep 15, 2023

@iloveeclipse

The build completed.

I assume there are likely to be test failures, though those UA tests are passing for me...

image

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.

2 participants