search

accel-sim / accel-sim-framework

This is the top-level repository for the Accel-Sim framework.
https://accel-sim.github.io
Other
273 stars 105 forks source link

clang-format isn't being properly enforced #305

Closed barnes88 closed 1 week ago

barnes88 commented 1 month ago

I don't think the method setup in https://github.com/accel-sim/accel-sim-framework/pull/236 is properly enforcing clang-format, because the dev branch has already diverged quite a bit from the formatting (I ran it on the latest dev commit and there are quite a few changes https://github.com/barnes88/gpgpu-sim_distribution/tree/clang-format-6.4.24). This is annoying if you have your editor set to enforce formatting on save, because it causes a bunch of lines to be changed that you didn't edit.

Maybe we can have Jenkins run a formatting stage diff so we can reject PRs that aren't properly formatted. Or we need everyone in the lab to install the pre-commit hooks.

@FJShen

tgrogers commented 1 month ago

Yeah we used to have a format checker. Please flag junrui to resurrect it. We have move to GitHub ci and away from maintaining our own Jenkins server.

On Tue, Jun 4, 2024 at 14:26 Aaron Barnes @.***> wrote:

I don't think the method setup in #236 https://github.com/accel-sim/accel-sim-framework/pull/236 is properly enforcing clang-format, because the dev branch has already diverged quite a bit from the formatting (I ran it on the latest dev commit and there are quite a few changes https://github.com/barnes88/gpgpu-sim_distribution/tree/clang-format-6.4.24). This is annoying if you have your editor set to enforce formatting on save, because it causes a bunch of lines to be changed that you didn't edit.

Maybe we can have Jenkins run a formatting stage diff so we can reject PRs that aren't properly formatted. Or we need everyone in the lab to install the pre-commit hooks.

@FJShen https://github.com/FJShen

— Reply to this email directly, view it on GitHub https://github.com/accel-sim/accel-sim-framework/issues/305, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7UY4NLUNNOGKXO4B4WRSDZFYBFLAVCNFSM6AAAAABIZBLXCKVHI2DSMVQWIX3LMV43ASLTON2WKOZSGMZTIMJSG42TGOI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

JRPan commented 1 month ago

Or I can spawn a job to run the format script before merge.

FJShen commented 1 month ago

Sorry for your inconvenience. Accel-Sim PR #236 (and GPGPU-Sim PR 60, which is still being reviewed) was an one-shot commit that re-formatted the code. The configuration file (.pre-commit-config.yaml) for "pre-commit" is included in the PR, but so far we don't have anything automated.

FJShen commented 2 weeks ago

@JRPan do you have anything implemented for the automatic formatting?

JRPan commented 2 weeks ago

Should we do automatic formatting before merging? Or reject non-formatted PR and ask people to format it?

barnes88 commented 2 weeks ago

I think having automatic formatting would be easiest, that way people don't have to worry about trying to match the version of clang-format