Skip to content
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

the npm clang-format package is unmaintained and obsolete #50157

Closed
anonrig opened this issue Oct 12, 2023 · 9 comments · Fixed by #53957
Closed

the npm clang-format package is unmaintained and obsolete #50157

anonrig opened this issue Oct 12, 2023 · 9 comments · Fixed by #53957
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.

Comments

@anonrig
Copy link
Member

anonrig commented Oct 12, 2023

We use clang-format npm package for running make format-cpp commands, and it seems like clang-format package by angular is unmaintained.

It was added to Node.js project at this commit: 0da144f#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52

Ref: angular/clang-format#82

@aduh95
Copy link
Contributor

aduh95 commented Oct 12, 2023

I guess a solution would be to compile it to WASM ourselves

@joyeecheung
Copy link
Member

I think in the short term it's fine since we use a fixed version of the npm package and we don't update the clang-format rules often. In the long term, compiling it to WASM or deferring to system-wide installation to a fixed version can be considered. Not sure when/how it was added to the CI but I think it would be fine removing it from the CI if using an old version ends up being problematic & we can't come up with a better solution - then we just go back to suggesting whitespace changes manually in the code reviews and hinting that clang-format vx.y can be used for those who don't know.

@tniessen tniessen added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. labels Oct 12, 2023
@e-dant
Copy link

e-dant commented Oct 13, 2023

I'm not too big into wasm, but I'm very curious why people keep mentioning it here. Clang, and all of its tools, rely quite a bit on parallel execution and the file system. Have things changed in wasm so far that either of those are possible?

I'm also not too into the workings and preferences of node, but I'm confused about why an npm package -- which appears to just build clang-format from source and layer on a cli-to-the-cli -- was used.

What am I missing? Why can't the actual clang-format package be used?

Edit: To be clear, I'm just curious. I don't mean to sound snarky.

@joyeecheung
Copy link
Member

joyeecheung commented Oct 13, 2023

I have not looked into clang-format so I don’t know if it needs any parallel file execution, but I doubt it needs anything very sophisticated as it’s just a formatting tool. Technically it just works on one file at a time and can also just read from stdin. We only use clang-format to format C++ code. We use the npm package because alas the llvm project does not distribute prebuilt clang-format binaries for multiple platforms (which is actually tiny) and the package does what we need - prebuild and distribute the binary for different platforms and download it during installation.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 15, 2023

The title here is a bit misleading. clang-format in itself is not unmaintained (far from it, it's probably one of the most used formatter on earth, I am fairly sure some big companies are spending money in its maintenance). It's just the npm package that ships pre-built binaries is no longer maintained.

I checked the Internet a bit and can't seem to find any other credible & versioned pre-built binaries. VScode's C++ plugin has one that's maintained by Microsoft but the download links aren't versioned. Chromium/V8 uses another one (documentation) maintained by Google and also the download links aren't very well versioned (although, theoretically we can still use the links in certain version of depot_tools's DEPS file, but it doesn't feel quite right to piggyback like this :)). llvm has prebuilt binaries but I am not sure which one of these archives contain clang-format and even if they do, they are too big - clang-format itself is tiny. It can be easily built from source https://github.com/llvm/llvm-project/tree/main/clang/tools/clang-format

Maybe something we can look into is setting up a GitHub repo with a workflow that just builds https://github.com/llvm/llvm-project/tree/main/clang/tools/clang-format and uploads the binary as releases that make lint-cpp-build in core can pull from.

@joyeecheung joyeecheung changed the title clang-format is unmaintained and obsolete the npm clang-format package is unmaintained and obsolete Nov 15, 2023
@joyeecheung
Copy link
Member

joyeecheung commented Nov 15, 2023

Another possible solution is to use some script that install a certain version of clang-format for different platforms. Most Linux distros provide some package that contains clang-format and on macOS there are e.g. homebrew formulas. Question is how do we ensure that the exact same version is installed for different platforms because different versions of clang-format can produce different results.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 15, 2023

I also found this https://github.com/muttleyxd/clang-tools-static-binaries which contains a workflow to build and release the binaries for Linux, macOS and Windows (it even builds all the versions from clang 3.9 to 17, and contains other clang tools). Maybe we can use the releases there or, if we are not too comfortable with using a third-party one, we can just fork it and build our own.

I think for us, the latest stable release of clang-format is enough. Though it would also be interesting to include the other tools and e.g. replace cpplint with clang-tidy (which can do a lot more in-depth linting and has a handy --fix)

@targos
Copy link
Member

targos commented Apr 14, 2024

FWIW I just forked the clang-format repo and I'm looking into creating builds using GH actions: https://github.com/targos/clang-format/tree/test-build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants