Strictly name the org.json module for JLink and JPackage #4

GedMarc · 2019-08-08T11:08:23Z

Enable JPMS for Library

Just until BGehrels#3 gets done

johnjaylward · 2019-08-20T15:47:58Z

I've not used maven in a long while, nor have experience setting up modules, but from what I've learned so far this is straightforward and should work as expected.

In short, if @BGehrels is OK with these changes, I see no reason to hold them back. @stleary do you have any direct feedback?

stleary · 2019-08-20T16:02:02Z

@GedMarc @johnjaylward @BGehrels No objection on my part.
Let's get on the same page regarding releases. If any valid user complaints emerge, I would ask that the release be immediately replaced with a new release that does not contain this PR.

GedMarc · 2019-08-20T16:59:08Z

@stleary @johnjaylward @BGehrels
Thank you for the updates, I have updated the module-info to be placed inside the multi-release location for backwards compatibility concerns.

I believe this is a good release candidate.

GedMarc · 2019-08-27T08:02:27Z

......

GedMarc · 2019-09-06T08:50:55Z

whatever

johnjaylward · 2019-09-06T16:05:21Z

@BGehrels @stleary can you comment on these changes? Are they approved to go? Are there any corrections you can see that need to be made? It's disappointing that @GedMarc's contributions seem to have been ignored again.

BGehrels · 2019-09-08T11:09:44Z

@GedMarc I shit, i'm sorry, haven't seen or expected this PR, notifications were drowning in my Github mailfilter, i'll have a look at them!

BGehrels · 2019-09-08T11:13:03Z

package.sh

+#mkdir -p $MODITECT_FOLDER
+# Copy the module-info to the dist folder to build
+#cp src/moditect/module-info.java $MODITECT_FOLDER/module-info.java
+#echo "Copied the module info file"


What about these 4 lines? Are they neccessary, not neccessary or optional? Or are there conditions when they should be enabled?

BGehrels · 2019-09-08T11:45:52Z

Did you test these changes with Java 8 and Java 9 in a Module/No-Module Scenario?

BGehrels · 2019-09-08T11:52:31Z

pom.xml

+                        <configuration>
+                            <!-- Put module-info.class in META-INF/versions/9 to maximize backwards compat -->
+                            <!-- See: https://github.com/moditect/moditect/issues/67 -->
+                            <jvmVersion>9</jvmVersion>


Will this automatically be available for Java 10, 11 and 12 then?

BGehrels · 2019-09-08T12:01:36Z

pom.xml

 				<configuration>
 					<source>%%JAVAVERSION%%</source>
 					<target>%%JAVAVERSION%%</target>
+                    <!--<source>1.6</source>
+                    <target>1.6</target>-->


With this, target is now defined twice

ahh, no, sorry, it's a block comment...

Could you remove the commented out code here?

BGehrels · 2019-09-08T12:03:35Z

pom.xml

+                        <artifactId>asm</artifactId>
+                        <version>7.0</version>
+                    </dependency>
+                </dependencies>


Could you add a comment to the pom why this new dependency is needed here?

BGehrels · 2019-09-08T12:08:29Z

pom.xml

@@ -97,7 +97,19 @@
 		</dependency>
 	</dependencies>

-	<build>
+    <!-- Allow finding fixed moditect version -->


Could you please extend this comment in a way that it somehow says when the snapshot repository can be removed again? Something like: "Remove this when we have an official moditect release containing bugfix X". Or "Remove this when modditect version x is released"?

jordanamr · 2019-12-27T11:25:10Z

No news from PR author ? :(

GedMarc · 2019-12-27T11:28:07Z

@jordanamr unfortunately a lot of these reviews are subject to the actual build cycle and depending on how the build and deploy happens, depends on what the correct implementation is.

the module-info file and source itself however are fully intact and being used pretty much everywhere for me - 4 production clients

https://search.maven.org/artifact/com.guicedee.services/json

GedMarc · 2019-12-27T11:29:10Z

Although the version is changing (with the encapsulating guicedee project) it is fully in-sync with source

stleary · 2020-01-04T17:09:34Z

@GedMarc can you please respond to each review comment? Once @BGehrels is satisfied the PR is acceptable, it is OK with me to proceed with a release. However, if any users complain that this breaks their build, the changes will have to be backed out and a new release made.

GedMarc · 2020-01-04T18:17:07Z

@stleary it's been 2 years with two of these, we've already released 1.0 and have decided to not include anything below jdk 8 support and have standardized on jdk 11
the changes are very basic and with a bit of effort and perhaps a test on the build with jdk 6 shouldn't take long at all to resolve anything that is incorrect,
not really keen on going back to this branch.

Thanks.

GedMarc added 11 commits November 26, 2018 22:05

org.json JPMS Module Naming

eb16f22

JPMS Build for org.json

401e4b1

Just until BGehrels#3 gets done

Update the version to build for pom, 1.6

5ea59dd

remove jre 11 dev tests

079e369

Javadoc errors generated

fd57ae4

Remove Nexus

793809b

weird, where did that go

e4d9aac

Update for PR

7356193

Update for PR

c54e16e

Update for PR

1c85fe1

Update for PR

b14459a

johnjaylward mentioned this pull request Aug 14, 2019

org.json JPMS Module Naming #3

Closed

Place moditect file into backwards compat safe place

a99329b

Put %%JAVAVERSION%% back for tests/script

6f63406

GedMarc closed this Sep 6, 2019

BGehrels reviewed Sep 8, 2019

View reviewed changes

BGehrels reopened this Sep 8, 2019

BGehrels reviewed Sep 8, 2019

View reviewed changes

GedMarc mentioned this pull request Jan 4, 2020

I would like to strictly name the module stleary/JSON-java#451

Closed

GedMarc mentioned this pull request Apr 25, 2020

Make use of module-info.java stleary/JSON-java#513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strictly name the org.json module for JLink and JPackage #4

Strictly name the org.json module for JLink and JPackage #4

GedMarc commented Aug 8, 2019

johnjaylward commented Aug 20, 2019 •

edited

Loading

stleary commented Aug 20, 2019

GedMarc commented Aug 20, 2019

GedMarc commented Aug 27, 2019

GedMarc commented Sep 6, 2019

johnjaylward commented Sep 6, 2019

BGehrels commented Sep 8, 2019

BGehrels Sep 8, 2019

BGehrels commented Sep 8, 2019

BGehrels Sep 8, 2019 •

edited

Loading

BGehrels Sep 8, 2019

BGehrels Sep 8, 2019

BGehrels Sep 8, 2019

BGehrels Sep 8, 2019

jordanamr commented Dec 27, 2019

GedMarc commented Dec 27, 2019

GedMarc commented Dec 27, 2019

stleary commented Jan 4, 2020

GedMarc commented Jan 4, 2020 •

edited

Loading

Strictly name the org.json module for JLink and JPackage #4

Are you sure you want to change the base?

Strictly name the org.json module for JLink and JPackage #4

Conversation

GedMarc commented Aug 8, 2019

johnjaylward commented Aug 20, 2019 • edited Loading

stleary commented Aug 20, 2019

GedMarc commented Aug 20, 2019

GedMarc commented Aug 27, 2019

GedMarc commented Sep 6, 2019

johnjaylward commented Sep 6, 2019

BGehrels commented Sep 8, 2019

BGehrels Sep 8, 2019

Choose a reason for hiding this comment

BGehrels commented Sep 8, 2019

BGehrels Sep 8, 2019 • edited Loading

Choose a reason for hiding this comment

BGehrels Sep 8, 2019

Choose a reason for hiding this comment

BGehrels Sep 8, 2019

Choose a reason for hiding this comment

BGehrels Sep 8, 2019

Choose a reason for hiding this comment

BGehrels Sep 8, 2019

Choose a reason for hiding this comment

jordanamr commented Dec 27, 2019

GedMarc commented Dec 27, 2019

GedMarc commented Dec 27, 2019

stleary commented Jan 4, 2020

GedMarc commented Jan 4, 2020 • edited Loading

johnjaylward commented Aug 20, 2019 •

edited

Loading

BGehrels Sep 8, 2019 •

edited

Loading

GedMarc commented Jan 4, 2020 •

edited

Loading