Skip to content

[EMAIL-203] Jakarta 2#80

Closed
KroArtem wants to merge 5 commits intoapache:masterfrom
KroArtem:EMAIL-203-jakarta-2
Closed

[EMAIL-203] Jakarta 2#80
KroArtem wants to merge 5 commits intoapache:masterfrom
KroArtem:EMAIL-203-jakarta-2

Conversation

@KroArtem
Copy link

@KroArtem KroArtem commented Jun 8, 2022

A continuation of PR #37

@KroArtem
Copy link
Author

KroArtem commented Jun 8, 2022

@garydgregory , can you please trigger workflows? I've rebased #37 on top of current master and fixed some small things in c54cc45

@jochenw
Copy link
Contributor

jochenw commented Jun 8, 2022

Can we have a TODO, or whatever, that suggests to restore the default value for build/defaultGoal after the 2.0 release, please?

@KroArtem
Copy link
Author

KroArtem commented Jun 8, 2022

Can we have a TODO, or whatever, that suggests to restore the default value for build/defaultGoal after the 2.0 release, please?

IDK, maybe it's possible to release with the checks being red, actually there are more failures coming before that :)

@jochenw
Copy link
Contributor

jochenw commented Jun 8, 2022

Can we have a TODO, or whatever, that suggests to restore the default value for build/defaultGoal after the 2.0 release, please?

IDK, maybe it's possible to release with the checks being red, actually there are more failures coming before that :)

A simple comment in the POM? Not much work for you, and visible in the Eclipse Task view.

@KroArtem
Copy link
Author

KroArtem commented Jun 8, 2022

Can we have a TODO, or whatever, that suggests to restore the default value for build/defaultGoal after the 2.0 release, please?

IDK, maybe it's possible to release with the checks being red, actually there are more failures coming before that :)

A simple comment in the POM? Not much work for you, and visible in the Eclipse Task view.

Sorry, sure, I've misread the message. Will do.

@Deprecated
public class ByteArrayDataSource implements DataSource
{
{
Copy link
Member

Choose a reason for hiding this comment

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

Reduce the noise in this PR please.

Copy link
Author

Choose a reason for hiding this comment

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

Done

pom.xml Outdated
<groupId>org.slf4j</groupId>
<artifactId>slf4j-jdk14</artifactId>
<version>1.7.7</version>
<version>1.7.30</version>
Copy link
Member

Choose a reason for hiding this comment

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

Don't do this in this PR, keep changes to what is direclty related, which this... not?

Copy link
Author

Choose a reason for hiding this comment

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

IDK what was the reason to update it (it was not my commit :)), reverted it back

@KroArtem
Copy link
Author

KroArtem commented Jun 8, 2022

I think that current maven.yml misses setting a profile for launches on java >=9

Launching mvn -P java17 -V -Ddoclint=all --file pom.xml --no-transfer-progress locally makes build pass, without P java17 it fails.

@KroArtem
Copy link
Author

KroArtem commented Jun 9, 2022

I see that java17 profile should be activated if jdk >=17 as per https://maven.apache.org/guides/introduction/introduction-to-profiles.html#details-on-profile-activation

JDK is indeed >=17 https://github.com/apache/commons-email/runs/6800894700?check_suite_focus=true#step:5:9

But it might be some Maven versions comparison magic that breaks here

@michael-o
Copy link
Member

Doing this will break all containers using still the javax namespace for the next 10 years. I have complained about the same done in Logback. Blindly migrated. Unless you prove that it cannot and will not break using a mail session from JNDI I am vetoing against this PR.

@KroArtem
Copy link
Author

KroArtem commented Jun 9, 2022

Doing this will break all containers using still the javax namespace for the next 10 years. I have complained about the same done in Logback. Blindly migrated. Unless you prove that it cannot and will not break using a mail session from JNDI I am vetoing against this PR.

This is what I was afraid of and that's the reason I've asked if there were any circumstances not to merge the previous PR.

There is no information about it in JIRA, no comments here in PRs and no discussions in mailing list. @michael-o , please copy your answer to the ticket for future generations, it may save some time for them.

@michael-o
Copy link
Member

FTR: https://jira.qos.ch/browse/LOGBACK-1575

@KroArtem
Copy link
Author

KroArtem commented Jun 9, 2022

Also bear in mind that jakarta is already in master it's just there was no release of it yet

I don't say my suggestion was right, it's just there were no evidence (in this project) that it shouldn't be done like this.

@michael-o
Copy link
Member

Also bear in mind that jakarta is already in master it's just there was no release of it yet

I don't say my suggestion was right, it's just there were no evidence (in this project) that it shouldn't be done like this.

You need to distinct was jakarta means. There are two aspects:

  • New Maven coordinates, old interfaces: fine to move,
  • New Java packages, new interfaces: will break everything.

@KroArtem
Copy link
Author

KroArtem commented Jun 9, 2022

True.

@garydgregory
Copy link
Member

-1 as it stands because this kind of work can only be done in the context of a major version change which means:

  • The package name changes from org.apache.commons.mail to org.apache.commons.mail2
  • The Maven artifacts changes from commons-email to commons-email2

@KroArtem
Copy link
Author

KroArtem commented Jun 9, 2022

Yeah, I have no objections closing the PR, at least now we have the reasoning saved here and in Jira

@KroArtem KroArtem closed this Jun 9, 2022
@reda-alaoui
Copy link
Member

-1 as it stands because this kind of work can only be done in the context of a major version change which means:

  • The package name changes from org.apache.commons.mail to org.apache.commons.mail2
  • The Maven artifacts changes from commons-email to commons-email2

@garydgregory , if I was to open the same PR as the current one with the points above fixed, would it be merged?

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.

6 participants