Skip to content

chore: update eslint #1789

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

Closed
wants to merge 29 commits into from
Closed

Conversation

CommanderStorm
Copy link
Collaborator

@CommanderStorm CommanderStorm commented Apr 12, 2025

Our frontend dependency on eslint is a bit dated. This PR migrates them to newer versions.

Eslint was changed to their new configuration format.
Bumping eslint requires bumping node requires bumping cross (due to libc)

Splitting this into this and #1792 is likey a good idea

Update:

All non-eslint parts of this PR will be broken out.
This is a hastle as is - as proven by the 29 commits.

@Copilot Copilot AI review requested due to automatic review settings April 12, 2025 20:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • martin/martin-ui/package.json: Language not supported

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! At some point we should rework CI a bit so it can auto-merge demo things without admin approval - i.e. make it so that the final CI task would pass if dependent jobs are skipped

@CommanderStorm CommanderStorm changed the title chore: modernise the frontends dependencys chore: bump eslint and vite frontend dependencys Apr 12, 2025
@CommanderStorm CommanderStorm marked this pull request as draft April 13, 2025 21:48
Cross.toml Outdated
"fnm default 22",
# verify installation
"node --version",
"npm --version",
Copy link
Collaborator Author

@CommanderStorm CommanderStorm Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to bump

  • node to next major as required by eslint
  • Bumping node requires newer libc
  • newer libc requiring cross update
  • cross update fucks with node in a major way 😮‍💨

But npm is still not quite installed in the next step (in the build script). Unsure where this is breaking apart.

Will need to look into how cross is really working.
maybe a symlink is missing, maybe more.

@CommanderStorm CommanderStorm changed the title chore: bump eslint and vite frontend dependencys chore: bump eslint, node and cross dependencys Apr 14, 2025
@CommanderStorm
Copy link
Collaborator Author

@nyurik @sharkAndshark I am a bit fed up with cross.

What would you think about a "boring" docker build via zigbuild?
=> in one github runner -> slower.
https://medium.com/@vladkens/fast-multi-arch-docker-build-for-rust-projects-a7db42f3adde

Would gating all-arch docker builds on main and for the others building and testing separate images be an option?

@nyurik
Copy link
Member

nyurik commented Jun 10, 2025

well, first thing first - lets not use cargo install cross --git https://github.com/cross-rs/cross for production build - we have no idea what the main branch of that repo contains, so should not be relied on. Especially without the --locked param. Combining both cross and npm update in one PR is also a bit dicey in my book. Lastly - I seriously seriously doubt we can do better than the big team with lots of history that built and maintains cross - they have far more expertise with it, and at the end of the day they use the same docker method. So best to figure out how to be able to take advantage of their knowledge

@CommanderStorm
Copy link
Collaborator Author

CommanderStorm commented Jun 10, 2025

Well if 0.3 were released, then choosing --git would not be a discussion. Also the momentum around this has sort of stalled.
I can do --ref .. --locked to still get the newer libc. (Except if the pinned non-repro bug hits and breaks this part -> cross-rs/cross#724).

Troubleshooting cross is a thorough pain.

No clue what it does or why it fails.
It is an arbitrary blob and does not explain what it does in their docs.

  • I remove a folder (/target) in the pre build step? It is there again.
  • I chown another (/.npm) to not be sudo? It is sudo again
  • I remove all cargo or docker caches? Fuck you, we cache somewhere and don't allow you to clean this part. (cross clean, cargo clean, docker system prune -a, docker stop $(docker ps -qa) && docker rm $(docker ps -qa) && docker volume rm $(docker volume ls -q))

I would hope that the docker route would at least be a reproducible build.
If I have to debug the cross code to get it to barely work, I could as well do it properly.

@nyurik
Copy link
Member

nyurik commented Jun 10, 2025

heh, i hear you, it does sound like a major pain. My only concern is that I do not want us to own even bigger part of the build process if we can avoid it... and yes, that's a big "if". I did spend a lot of time dealing with cross a while back, and was hoping to never learn more of it... alas

Looking at their repo, it is still fairly active, and the number of test targets and the complexity of testing overall is scary - which is what i would hope to avoid

@CommanderStorm CommanderStorm changed the title chore: bump eslint, node and cross dependencys chore: update eslint Jun 10, 2025
@CommanderStorm
Copy link
Collaborator Author

#1909 replaces this

@CommanderStorm CommanderStorm deleted the frontend-bump1 branch July 10, 2025 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants