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: compatible with Maven 4 #9

Merged
merged 5 commits into from
Sep 24, 2024
Merged

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Sep 23, 2024

@tisonkun
Copy link
Owner

@gnodet Thank you! I'm going to integrate with you changes, merge and release.

But still, I wonder how the removed part are "supporting Maven 4", so that I may pick up this topic later to deliver a good integration :D

@gnodet
Copy link
Contributor Author

gnodet commented Sep 24, 2024

@gnodet Thank you! I'm going to integrate with you changes, merge and release.

But still, I wonder how the removed part are "supporting Maven 4", so that I may pick up this topic later to deliver a good integration :D

The PropertyContributor is new to the Maven 4 API, so it can only be loaded if running in Maven 4, in which case, to avoid running the code twice, it disables the Maven 3 extension. The idea was that the internals may change in Maven 4 in the future and the Maven 3 extension may not work anymore with Maven 4 in the future.
However, things have changed a lot since I worked on that, and the Maven 4 extension should be rewritten anyway.

@tisonkun
Copy link
Owner

the Maven 4 extension should be rewritten anyway.

Yes. As long as Maven 4 is released or the API is relatively stable, and there are enough docs to start with, I'd write a plugin-maven-4 for it.

@tisonkun
Copy link
Owner

OK. Then Let me merge and release this patch today. Thanks again for your information.

@tisonkun tisonkun changed the title patch maven 4.0 fix: compatible with Maven 4 Sep 24, 2024
@tisonkun tisonkun marked this pull request as ready for review September 24, 2024 03:28
@tisonkun tisonkun merged commit 6c8f160 into tisonkun:main Sep 24, 2024
3 checks passed
@tisonkun
Copy link
Owner

I found that this is actually a fix. So I publish 0.3.1 for it.

@linghengqian you can try to replace the os-maven-plugin with:

    <build>
        <extensions>
            <extension>
                <groupId>com.tisonkun.os</groupId>
                <artifactId>os-detector-maven-plugin</artifactId>
                <version>0.3.1</version>
            </extension>
        </extensions>
    </build>

    <dependencies>
        <dependency>
            <groupId>org.apache.opendal</groupId>
            <artifactId>opendal</artifactId>
            <version>${opendal.version}</version>
        </dependency>
        <dependency>
            <groupId>org.apache.opendal</groupId>
            <artifactId>opendal</artifactId>
            <version>${opendal.version}</version>
            <classifier>${os.detected.classifier}</classifier>
        </dependency>
    </dependencies>

The same user experience.

@tisonkun
Copy link
Owner

Before release a 1.0, I'd try to find some time consider #6 and #7. And always try to seek the opportunity to work with both Trustin and Google (the upstream) so that we merge the community instead of fork another ..

@linghengqian
Copy link

@linghengqian you can try to replace the os-maven-plugin with:

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.

3 participants