code-server icon indicating copy to clipboard operation
code-server copied to clipboard

[Chore]: investigate caching in platform build steps

Open jsjoeio opened this issue 2 years ago • 3 comments

What is your suggestion?

to utilize caching in the platform build steps

Why do you want this feature?

speed up CI

Are there any workarounds to get this functionality today?

no

Are you interested in submitting a PR for this?

i will if no one else beats me to it

jsjoeio avatar Sep 09 '22 20:09 jsjoeio

I am interested in helping, however I'm still relatively new so I prob need a few pointers. Where should I start looking for places for potential caching?

ChiatzenW avatar Sep 12 '22 15:09 ChiatzenW

@ChiatzenW thanks for offering to help! I would start here. This is a successful run of Build and it took 32m.

If you look through each job, you can see the total time for each. The ones over 5m are probably the ones that have areas where we can do some caching, specifically the macOS and Linux ones.

Let me know if you want more pointers!

jsjoeio avatar Sep 12 '22 18:09 jsjoeio

I'm guessing we cannot skip the tests? As of current I see that various tests take up majority of the time and looks like aside from that we have the build process that seems to take up some time. Shall I work on eliminating building unnessecary parts where we can just use the last version?

ChiatzenW avatar Sep 13 '22 01:09 ChiatzenW

I'm guessing we cannot skip the tests? As of current I see that various tests take up majority of the time and looks like aside from that we have the build process that seems to take up some time.

Yes, we cannot skip the tests. However...we could only run the unit tests when there are changes to *.ts files. That's one small improvement. The e2e tests should only be run if there are changes to *.ts or lib/vscode or patches/*.

Shall I work on eliminating building unnessecary parts where we can just use the last version?

That sounds like a great idea! Before you do any work, can you share what you plan to do for this so we can make sure we're aligned?

jsjoeio avatar Sep 14 '22 17:09 jsjoeio

hmm for tests I'm thinking maybe keep a variable somewhere and modify it every time tsc -w compiles? Or just calculate a SHA256 for all *.ts file and check them before we run the e2e tests.

As for building I'm thinking check if the package is already present and up-to-date and only install them when they are not? I'm also curious if we can implement kind of like a Make-like system as I believe tsc -w --extendedDiagnostics can show files that triggered the recompilation fot the typescript part of the project and reduce the number of release artifacts we need to upload. I think the other parts like Code itself should be fairly well separated? So maybe we can keep them as is if only other things are modified. As of current building VScode and extensions takes 7m56s.

ChiatzenW avatar Sep 14 '22 18:09 ChiatzenW

That could work! We do something similar in coder/coder using a GitHub Action which might be a simpler approach: https://github.com/coder/coder/blob/main/.github/workflows/coder.yaml#L59-L76

I believe tsc -w --extendedDiagnostics can show files that triggered the recompilation fot the typescript part of the project and reduce the number of release artifacts we need to upload

That's an interesting idea! I wonder though if we still want to upload artifacts every time? Maybe going the cache build dependencies approach might be a better first step. Thoughts?

cc @code-asher in case you have any thoughts?

jsjoeio avatar Sep 14 '22 20:09 jsjoeio

I looked into caching dependencies and it seems that it is already documented in the github actions documentation. Should I try following the steps here?

ChiatzenW avatar Sep 15 '22 00:09 ChiatzenW

@ChiatzenW great find! Yes, I'd say that seems like a good first step. Look at ci.yaml and see which steps could benefit from caching 👍🏼

jsjoeio avatar Sep 15 '22 15:09 jsjoeio

Yeah I think the platform build steps (like the x86-64 Linux build step) are the only ones missing NPM dependency caching. We can copy a cache step from one of the other jobs into the platform jobs then add the platform to the key.

We can probably skip the nfpm step for all runs except the release run. We could filter it out based on the tag or put it in a separate workflow. We will eventually need a separate workflow once we refactor the release anyway.

I like the idea of skipping e2e tests when nothing changes. I think the ideal way to do this is to create a separate workflow that depends on the build workflow and use the paths or paths-ignore filters.

I forget; did we test using multiple workers on the e2e tests? That might speed things up.

code-asher avatar Sep 15 '22 21:09 code-asher

I like those suggestions!

I forget; did we test using multiple workers on the e2e tests? That might speed things up.

I think so and I don't think it had any effect :/ Could be worth trying again. Previous attempts: https://github.com/coder/code-server/issues/5435#issuecomment-1223056105

jsjoeio avatar Sep 15 '22 22:09 jsjoeio

I have checked with the ci.yaml file and indeed the x86-64 Linux build step is not using cache. However it says in the comments above that

this requires refactoring our release scripts

Is applying caching similar to cache-code OK or is there any further scripts that needs to be changed?

ChiatzenW avatar Sep 16 '22 14:09 ChiatzenW

Ahh I see the comment:

https://github.com/coder/code-server/blob/main/.github/workflows/ci.yaml#L214-L216

I think we can ignore for now.

Is applying caching similar to cache-code OK or is there any further scripts that needs to be changed?

Yes, it should be! If you get stuck, let us know :)

jsjoeio avatar Sep 16 '22 15:09 jsjoeio

Ah somehow I missed that table! Very helpful. I suppose that means we are already maxing out our resources or that individual tests are taking a very long time.

Oh yeah I remember that "requires refactor" comment now. I think we can delete it. Originally we were going to take a different approach to caching that required modifying the release scripts but I think it is better to just use the caching action.

code-asher avatar Sep 16 '22 17:09 code-asher