ci: remove useless cargo hack check from ci/test-check.sh

[yihau]

Problem

(split from #9651)

we don't run any cargo hack related commands in ci/test-check.sh

Summary of Changes

remove the check

[mircea-c]

Doesn't test-check.sh just verify that tools are installed correctly for other tests to use? I don't know that it needs to be used in this script in particular.

[yihau]

Doesn't test-check.sh just verify that tools are installed correctly for other tests to use? I don't know that it needs to be used in this script in particular

my ultimate goal is to get test-check.sh standalone. I don't think it's a good idea to check a tool which is not used in this script.

[yihau]

also, I think this check is redundant. if the installation is incorrect, the corresponding script should fail anyway🙈

[steviez]

also, I think this check is redundant. if the installation is incorrect, the corresponding script should fail anyway🙈

I'd have to refresh myself on where cargo hack gets used eventually, but I could see checking for installation of tools early in the CI run as a "fail fast" mechanism. Ie, it would be a waste to spawn a bunch of jobs in parallel and run for an hour only to fail b/c one of the last steps doesn't have the right tool available.

That being said, test-checks looks like it is "checking user code" while the cargo hack check is "verifying CI environment". So, if I didn't miss the mark with my "fail fast" comment and there is value in keeping that, maybe it could/should be another script like ci/test-ci-env.sh or something

[yihau]

this check was introduced in solana-labs#33519. it looks like the motivation was to improve the output.

I don't think we will hit this in our ci environment, since we already run most of our steps inside a docker image.

[mircea-c]

I don't think we will hit this in our ci environment

As per our discussion earlier today, users should be able to run the tests on their local machine. So I think it would be good to leave this in here.

[yihau]

As per our discussion earlier today, users should be able to run the tests on their local machine. So I think it would be good to leave this in here.

I think that exactly the point. when we run this script (ci/test-check.sh), we will check cargo hack. however, it never uses in this script. it would be very confusing if it failed because of a dependency that is never used

[steviez]

we don't run any cargo hack related commands in ci/test-check.sh

Looking around the repo, do we still use cargo hack anywhere ? I believe stuff that is now in the SDK did, but it isn't super obvious to me that we are still using it for anything in Agave today. My whole argument for keeping this check was based upon an assumption that we were using hack somewhere. Is there another script where we call hack ?

Let's chat about this on the devops call; I think there might be a misunderstanding that will be easier resolved on Zoom.

[yihau]

Is there another script where we call hack ?

dcou steps are still using it. I will use it in my new feature check step as well.

I'm still a bit confused why script B needs a dep but we use script A to check. I guess ci/test-ci-env.sh might be a better place to handle this. however, I expect it will become outdated easily

[mircea-c]

I think that these checks were put in here because this script runs first in the pipeline.

But this is also fine. If the CI fails when run locally, the error will be pretty obvious.

[steviez]

I guess ci/test-ci-env.sh might be a better place to handle this. however, I expect it will become outdated easily

Yeah, this was my thinking but fair that it could be prone to becoming outdated. In any case, I think we can proceed on this one adjust things back in the future if we need