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

Add test input java classes to jandex repository #101

Closed
vlsi opened this issue Jan 5, 2021 · 4 comments
Closed

Add test input java classes to jandex repository #101

vlsi opened this issue Jan 5, 2021 · 4 comments
Assignees
Milestone

Comments

@vlsi
Copy link
Contributor

vlsi commented Jan 5, 2021

Currently, it is hard to write tests since jandex repository defaults to source=target=1.6, and the test code that uses type annotations just does not compile.

It would be nice if there was a way to add test classes right into jandex repository (e.g. via Maven toolchains or Gradle toolchains)

@vlsi vlsi changed the title Add input java classes to jandex repository Add test input java classes to jandex repository Jan 10, 2021
@n1hility
Copy link
Contributor

For the moment you can add java 8 test classes here: https://github.com/wildfly/typeannotation-test

It has come time to move to 8, but this will probably mean declaring a 3.0 version.

@vlsi
Copy link
Contributor Author

vlsi commented Jan 11, 2021

For the moment you can add java 8 test classes here: https://github.com/wildfly/typeannotation-test

It feels weird to add classes in one place and debug them in another :-/
That would be super-weird for PR purposes as well.


I guess different modules can have different source/target versions if you still need to support Java 6/7.
Have you explored that possibility?

However, jandex issues hit Apache Calcite badly, and I guess it might be even easier to fix jandex than implement workarounds in Calcite :-/

On the other hand, I have no preference regarding 6-7-8 (Calcite is Java 8+ anyway), so if you migrate jandex to Java8, then it would indeed help with testing.

@n1hility
Copy link
Contributor

n1hility commented Jan 11, 2021

For the moment you can add java 8 test classes here: https://github.com/wildfly/typeannotation-test

It feels weird to add classes in one place and debug them in another :-/
That would be super-weird for PR purposes as well.

The test cases you debug are only expected to be in the main jadex project, it's just the input byteocde that is elsewhere,. The pattern is like this since the project is forward bytecode compatible (it can read newer bytecode than the JVM it runs on. E.g you can read Java 11 bytecode running on a Java 6 JVM). I agree its unusual, and means you have to build install in two separate steps.
.

I guess different modules can have different source/target versions if you still need to support Java 6/7.
Have you explored that possibility?

Historically, the only reason was to avoid requiring contributors to have extra ultra new JVMs around. So for example if we added early support for a pre-release of say Java 15, but you just wanted to fix something for the current stable VMs its a bit of a PITA.

However, jandex issues hit Apache Calcite badly, and I guess it might be even easier to fix jandex than implement workarounds in Calcite :-/

sorry about that!

On the other hand, I have no preference regarding 6-7-8 (Calcite is Java 8+ anyway), so if you migrate jandex to Java8, then it would indeed help with testing.

Yeah I think its a bit overdue to move to 8 as a minimum baseline. I'd be ok with exploring a multi-module interim step for at least bytecode through 11

@Ladicek
Copy link
Contributor

Ladicek commented Sep 7, 2021

This is now done on the smallrye branch.

@Ladicek Ladicek closed this as completed Sep 7, 2021
@Ladicek Ladicek self-assigned this Sep 7, 2021
@Ladicek Ladicek added this to the 3.0.0 milestone Sep 7, 2021
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

No branches or pull requests

3 participants