Skip to content

Remove weird coupling of antsrc and apisupport.ant sources #8480

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lkishalmi
Copy link
Contributor

Well, was checking why the NetBeans Snap build from sources does not build any more.

It fails with:

     [exec] :: -compile-copyap:
     [exec] ::     [mkdir] Created dir: /root/parts/build/build/apisupport/apisupport.ant/build/copyapclasses
     [exec] ::  [nb-javac] Compiling 1 source file to /root/parts/build/build/apisupport/apisupport.ant/build/copyapclasses
     [exec] ::  [nb-javac] Ignoring source, target and bootclasspath as release has been set
     [exec] :: [nb-ext-jar] Building jar: /root/parts/build/build/apisupport/apisupport.ant/build/copyap.jar
     [exec] ::
     [exec] :: init:
     [exec] ::
     [exec] :: -prepare-mandatory-files-for-module:
     [exec] ::     [mkdir] Created dir: /root/parts/build/build/apisupport/apisupport.ant/build/classes/META-INF
     [exec] :: [createlicensesummary] All tests passed
     [exec] ::
     [exec] :: up-to-date:
     [exec] ::
     [exec] :: -pre-compile:
     [exec] ::
     [exec] :: -do-compile:
     [exec] ::  [nb-javac] Compiling 153 source files to /root/parts/build/build/apisupport/apisupport.ant/build/classes
     [exec] ::  [nb-javac] Ignoring source, target and bootclasspath as release has been set
     [exec] ::    [repeat] /root/parts/build/build/apisupport/apisupport.ant/src/org/netbeans/modules/apisupport/project/Evaluator.java:63: error: package org.netbeans.nbbuild.extlibs does not exist
     [exec] ::    [repeat] import org.netbeans.nbbuild.extlibs.SetupLimitModulesProbe;
     [exec] ::    [repeat]                                    ^
     [exec] ::    [repeat] 1 error
     [exec] ::   [nbmerge] Failed to build target: all-apisupport.ant

So I was looking for the SetupLimitModulesProbe class and found PR #7201 .

As a seasoned build engineer I found this a weird way to solve some a classic class duplication issue without declaring dependencies.

I know that we are trained to do things DRY. Also annotation processors are on the blurred line of a build tool and a compiler.

Though in this case I think a well documented code duplication would be much cleaner solution.

@lkishalmi lkishalmi added this to the NB27 milestone May 2, 2025
@lkishalmi lkishalmi added the Ant [ci] enable "build tools" tests label May 2, 2025
@lkishalmi
Copy link
Contributor Author

BTW, this also solves the Snap build issue. I'll try to debug it a bit further, to find out why it is failing there, but nowhere else. Though Snap builds are running in a really bare/restricted environment.

@apache apache locked and limited conversation to collaborators May 2, 2025
@apache apache unlocked this conversation May 2, 2025
@neilcsmith-net
Copy link
Member

I know that we are trained to do things DRY ... Though in this case I think a well documented code duplication would be much cleaner solution.

+1 DRY is sometimes fundamentally overrated. On the other hand, I'd like to understand more about why it was done in this way.

It would also be good to get to the bottom of why the Snap build is behaving differently than other builds, because it could be a pointer to other configuration issues.

@mbien
Copy link
Member

mbien commented May 5, 2025

doesn't ant have a copy task somewhere? I think asm is also copied around so it might work with a source file too,

btw I see this class only in one jar:

sh search-jars.sh SetupLimitModulesProbe.class
searching for 'SetupLimitModulesProbe.class'
nbbuild/netbeans/apisupport/modules/org-netbeans-modules-apisupport-ant.jar
     7324  2025-05-02 23:35   org/netbeans/nbbuild/extlibs/SetupLimitModulesProbe.class

but maybe my script can't find it.

+1 DRY is sometimes fundamentally overrated.

I wouldn't say its overrated, but sometimes it is ok to break rules.

I mentioned during review that we could try to find a way to deduplicate the file but I hoped for something simpler tbh :) #7201 (comment)

@neilcsmith-net
Copy link
Member

btw I see this class only in one jar:
...
but maybe my script can't find it.

Probably. It's part of the nbbuild ant task sources. Maybe tasks.jar?

+1 DRY is sometimes fundamentally overrated.

I wouldn't say its overrated, but sometimes it is ok to break rules.

It's a principle not a rule, as are SoC, LoB, KISS, and countless other principles that are worth consideration while often conflicting with each other. When DRY introduces extra complexity and fragility, while also reducing comprehension, it is fundamentally overrated imo. I'm in agreement with Laszlo on this one.

@mbien
Copy link
Member

mbien commented May 5, 2025

it is fundamentally overrated imo

taking this repo as example I disagree with this phrasing. If you don't try to deduplicate you have to fix things multiple times. As someone who does project wide refactorings (e.g due to #8256) I might have more awareness to those issues than others e.g if I see the same piece of code 5 times in a diff - some of them with bug fixes. Or see the diff panel appear multiple times across NB with some having correct "diff to" ordering.

As I said before: if there is no easy way to share this file, fine lets take the lesser evil. But copy and paste is not a overrated problem in my opinion. Whole modules (#7958) were copied and had to be deduplicated again with extra effort. JShell integration injects copied JDK code etc.

I am ok with this PR but don't agree with the assessment.

@neilcsmith-net
Copy link
Member

@mbien agreed, and most of which would also reduce complexity and fragility. I'm not sure why you're having an argument with only one third of my sentence, but in doing so we're saying the same thing - "let's take the lesser evil"! 😆

@lkishalmi
Copy link
Contributor Author

Well, I've done some testing.

My original issue might be a weird Snapcraft one. It has a debug mode, which drops you into the container if the build is failing. Executing the same build command there from bash result a successful build. From that point, I'm bit lost.

Still, I think, this is the way to fix that issue.

@lkishalmi
Copy link
Contributor Author

If no one objects, I'm going to merge this in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ant [ci] enable "build tools" tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants