Open bramp opened 8 months ago
I have to admit, I haven't been using java day-to-day for many years now, but I'm curious the trend with immutability and public properties.
Today we have something like this:
public class FFmpegProbeResult {
public List<FFmpegStream> streams;
public List<FFmpegStream> getStreams() {
if (streams == null) return Collections.emptyList();
return Collections.unmodifiableList(streams);
}
}
Can we drop the getters, and just always do:
@Immutable
public class FFmpegProbeResult {
@NonNull
public ImmutableList<FFmpegStream> streams;
FFmpegProbeResult(List<FFmpegStream> streams) {
this.streams = ImmutableList.copyOf(streams == null ? Collections.emptyList() : streams);
}
}
i.e drop the getters, and make all fields public but immutable. Avoid the defensive copies on getter calls. Move to Guava's ImmutableList to ensure the list can never change.
Maybe we should write a best practice / style guide for what the APIs should look like.
Please dont remove java 8 support. For this library the "new" language features are pointless. They offer no functional improvement over what exists. No need to make the life of probably thousands of developers harder just for a bit of syntactic sugar.
While most companies use java 17 for new projects please dont abandon the poor people like me that have to maintain code bases that are older then we are ourselves. Due to the changes from java 8 to 9 there are countless projects that will NEVER make the jump from 8 to 11(or newer). The jump from 11 to 17 is easier, The jump from 17 to 21 is trivial. Most people that are still on 8 will not be able to migrate.
@AlexanderSchuetz97 My condolences.
How dependent on this library are you? My argument is that most project using java 8 and this library should be fairly established and shouldn't depend on newer features.
I agree that upgrading isn't strictly necessary, but would improve developer experience and maintainability in this project. I'm fine with keeping this project on a lower version for compatibility reasons.
Thanks for your input @AlexanderSchuetz97 ... if any jump does occur the old version of this library will still be supported.
One concern I do have, is many dependencies are moving off Java 8. Literally right now I'm trying to use java 8 to compile this project, and various maven plugins don't work, and the tests can't run (due to dependencies moving beyond 8). I think I can work around that, but at some point we will have to change.
However chances aren't we won't bump until we absolutely need to.
@bramp I would still opt for getters (or record style accessors). A lot of the tooling works better with getters/setters and encapsulation is better. It is more boiler plate (and I hate that as much as everyone else), but at least for me, the benefits outnumber the drawbacks.
The software I make/maintain is used in a medical environment where its used to transcode videos/pictures of patients. I was just looking on this repo on progress in regards to a CVE and just saw this. You are completely correct that such software is well estabilshed and I honestly If I could would never update the library. This is unfortunately not possible due to outside pressures.
Essentially my worry is that if you do remove java 8 support in a newer version and someone does find a vulnerabilty (be it a real one or a percieved one as the current one appears to be) someone will force me to do something about it eventually. If updating to a newer version is impossible due to no more updates for old java then I would essentially have to clone this library and start maintainig it myself. That or get rid of it entirely and do everything with ProcessBuilder.
As for build process, we use gradle but this is well possible with maven too. The trick is to run maven/gradle with a newer version of java but instruct the compiler to compile for an older version. I personally have many thingy I compile with maven running under jdk11 and compile for target 7.
Regarding java version: Even if we bump, we could still support a minimal java 8 build. It won't be equal to the master build, but fixes/some features could still be added.
@Euklios If you do want to you could build a multi version jar. Its what log4j does to resolve this issue. Essentially its a jar file with classes for different java versions. The JDK picks the class for its java version. I have never needed to setup such a build, but you could look at log4j for guidance on how to set this up. Personally If I was the maintained of this repo Id just stick to java 8. Its the pragmatic solution since the only gain at least as far as I am aware is syntactic sugar. You will also introduce compile errors into peoples projects since many projects (some of ours too) overload some of your methods and mess with the internals of this library. (Mainly for some timeout scenarios I believe?)
Is the effort of maintaining 2 branches one for j8 one for newer really worth it?
@AlexanderSchuetz97 You're not hiring by any chance? I'm already maintaining four forks of this library, what's one more? Jokes aside, even if we update, I'd still be willing to patch if required.
What I'm doing right now is keeping a private fork of this library and adding my changes to that fork first. Artifacts are pushed to a private maven registry, where my other projects pull from. The time-consuming part is syncing with upstream
If the cve is actually more than smoke, feel free to open an issue. I'd be happy to fix asap
The current CVE is smoke at least as far as I can tell. Jokes aside, I think that the next version of the library will not change anything in regards to the CVE but I doubt that anyone will resubmit the CVE so it will sort itself out naturally. If only one could teach automated vunerabilty scanners (that customers are using) that some CVE's are smoke my life would be much easier.
Anyways thanks for the nice chat and willingness to patch if necesarry. Its appreciated.
Anyways thanks for the nice chat and willingness to patch if necesarry. Its appreciated.
Don't worry, if it's something trivial we could still consider a fix. Maybe that would keep your customers away for a bit. Although executing shell commands is kinda our thing ^^
Is the effort of maintaining 2 branches one for j8 one for newer really worth it?
We're not log4j and I agree, it's most likely not worth it. My suggestion is a Java 8 build for people like you, with fixes and some small features where possible, while the master can develop on its own.
I'd like to add some bigger features that would greatly benefit from newer Java features, but they are not strictly required. I'd benefit more from "cleaning" the API, removing public properties, and so on.
FYI I just had to make these changes: https://github.com/bramp/ffmpeg-cli-wrapper/commit/053c32ce8086d431573113f135d18b65963d764e so this actually builds on JDK 8.
I forgot that the Java version before 9 was 1.8, not 8. I should have noticed...
@bramp I'll close #269 in favour of this one.
If your intentions regarding that issue are still the same, I'd be willing to take over a maintainer role if you want to hand off some responsibility. As long as I can close all the answered questions that are cluttering my view
I'm happy to add you as a maintainer, just don't do anything I wouldn't do :). For Java 9+ I'd like to understand the exact benefits before we break our customers.
That's fine. You fixed the build issues with Java 8, and as long as we don't plan to migrate soon, that doesn't block me.
Any interest in a port to Gradle, or would you like to keep the project on mvn?
@eygraber I'm not sure I know the advantages of gradle over maven, but I suspect since I would have to learn how to continue to maintain a gradle setup, this would make my life more difficult. Thus thanks, but I don't think it'll be good for the project.
Java 8 is 10 years old, these people crying about maintaining backward's compatibility have had 10 years to migrate their projects. I say let the asteroid hit the dinosaurs. 🦕 ☄️
As commented by @Euklios the java LTS are 8, 11, 17, 21, with "Java 8 was so popular that it won't die.", but many companies moving to active projects to java 17 or 21.
So I think we should plan for moving to the latest java (at some point) and at the same time, make appropriate changes to the API to modernise it. A quick shortlist (again by @Euklios):