This sounds good to me. Can we shorten 'build-project' to just 'build'? Kind Regards, Brandon
On Thu, Jun 29, 2023 at 3:22 AM Jacek Lewandowski <lewandowski.ja...@gmail.com> wrote: > > So given all the feedback, I'm going to do the following: > > "jar" will depend on "check" target > "build-project", "build-test" and "test" targets will not depend on "check" > target > "check" target will include checkstyle, rat and eclipse-warnings > > There is an additional flag "no-check" to disable checks in the "jar" target. > > Will not introduce any Git hook. > > wt., 27 cze 2023 o 18:35 Jacek Lewandowski <lewandowski.ja...@gmail.com> > napisał(a): >> >> With git you can always opt-out by adding --no-verify flag to either push or >> commit >> >> I really prefer separate tasks than flags. Flags are not listed in the help >> message like "ant -p" and are not auto-completed in the terminal. That makes >> them almost undiscoverable for newcomers. >> >> Want to have jar include checks? Ok, but let's don't run checks >> automatically with "build" or "test" >> >> >> >> wt., 27 cze 2023 o 18:26 David Capwell <dcapw...@apple.com> napisał(a): >>> >>> nobody referred to running checks in a pre-push (or pre-commit) hook >>> >>> >>> >>> In accord I added an opt-out for each hook, and will require such here as >>> well… as long as you can opt-out, its fine by me… I know I will likely >>> opt-out, but wouldn’t block such an effort >>> >>> Your point that pre-push hook might not be the best one is valid, and we >>> should rather think about pre-commit >>> >>> >>> Pre-push is far better than pre-commit, with pre-commit you are forcing a >>> style on people…. I for one have many many commits in a single PR, where I >>> use commits to not loose track of progress (even if the code doesn’t >>> compile), so forcing the build to work would be a -1 from me…. Pre-push at >>> least means “you want the world to see this” so makes more sense there… >>> >>> Again, must have an opt-out >>> >>> proposed: >>> ant jar (just build) >>> git commit (run some checks) >>> >>> >>> I am against this, jar should also check and ask users to opt-out if they >>> don’t want the checks…. >>> >>> If we go with opt-out i'd like to see one flag that can disable all checks: >>> `-Dchecks.skip` >>> >>> >>> Works for me, you can also do the following to disable and not worry about >>> >>> $ cat <<EOF > build.properties >>> checks.skip: true >>> EOF >>> >>> On Jun 27, 2023, at 3:14 AM, Mick Semb Wever <m...@apache.org> wrote: >>> >>> The context is that we currently have 3 checks in the build: >>> >>> - Checkstyle, >>> - Eclipse-Warnings, >>> - RAT >>> >>> >>> >>> And dependency-check (owasp). >>> >>> >>> >>> I want to discuss whether you are ok with extracting all checks to their >>> distinct target and not running it automatically with the targets which >>> devs usually run locally. In particular: >>> >>> >>> "build", "jar", and all "test" targets would not trigger CheckStyle, RAT or >>> Eclipse-Warnings >>> A new target "check" would trigger all CheckStyle, RAT, and Eclipse-Warnings >>> The new "check" target would be run along with the "artifacts" target on >>> Jenkins-CI, and it as a separate build step in CircleCI >>> >>> >>> >>> +0 I prefer this opt-in over an opt-out approach. >>> >>> It should be separated from `artifacts` too. >>> We would need to encourage engineers to run `ant check` before >>> starting CI and/or requesting review. >>> >>> I'm in favour of the opt-in approach because it keeps it visible. >>> Folks configure flags and it "disappears" forever. Also it's a >>> headache in all the other ant targets where we actually don't want it, >>> e.g. tests. >>> >>> If we go with opt-out i'd like to see one flag that can disable all >>> checks: `-Dchecks.skip` >>> >>> >>> That could be fixed by running checks in a pre-push Git hook. There are >>> some benefits of this compared to the current behavior: >>> >>> >>> >>> -1 >>> committing and pushing to a personal branch is often done to save work >>> or for cross-machine or collaboration. We should not gate on checks or >>> compilation here. >>> >>> PRs should fail if checks fail, to give newcomers clear feedback (and >>> to take this nit-picking out of the review process). >>> >>>
