search

guardian / typerighter

Even if you’re the right typer, couldn’t hurt to use Typerighter!
Apache License 2.0
276 stars 12 forks source link

Perform better checks in our start up script #384

Closed Fweddi closed 1 year ago

Fweddi commented 1 year ago

What does this change?

We now check for java and node versions in our start-up scripts. If nvm is installed, we switch node versions automatically.

The build.sbt file also checks for the java version - so we get the same wins if we run the code from within sbt.

There are also a couple of other quick wins around checking for PIDS before we kill them, and tearing down our dependencies in more fail situations.

Fweddi commented 1 year ago

I've addressed these comments by removing prescription for any particular manager. If you have fnm, it will try to run that. If you don't, or if fnm fails (as it does for me!), then it will try to use nvm. And if you have neither of those, you have the option of changing manually (using your version manager of choice).

I have also copied the checks into the main start script, passing a --checks-complete flag to the individual scripts. This means the checks are performed only once when running the main start script.

Fweddi commented 1 year ago

Works really nicely. I wonder how we'd scale this out to other tools, it'd be lovely if this were the standard everywhere (but difficult to maintain.)

A few non-blocking comments.

I think there's a hope here that we can copy these patterns to other tools. It would be great if we had a more standard way of doing this, which didn't involve dozens of bash scripts copied and pasted across repositories.

rtyley commented 1 year ago

Uh oh! An unfortunate unanticipated consequence of this change is that the sbt initialize setting added with this PR (checking the Java version) causes a job 'failure' for our Scala Steward process - which is running with Java 11:

java.lang.AssertionError: assertion failed: Java 8 is required for this project. You are using Java 11.

image

The error doesn't actually kill the Scala Steward process (other repos get updated successfully), but it does cause the entire job to be reported as a 'failure' (which is a bit noisy), and it's still ongoing (ie the last 3 weeks!).

Even worse, because the job 'fails' I think the GitHub Action is not caching, which has caused the execution time to gradually increase from 3 mins to 25 mins, and Scala Steward to forget when it last opened PRs for dependencies, leading to it offering more frequent updates than it should.

Some possible fixes:

Could you take a look at this @Fweddi ?

jonathonherbert commented 1 year ago

Thanks for this, Roberto! I've kicked off a bake for a Java 11 version of our ngrams-ready AMI here https://amigo.gutools.co.uk/recipes/editorial-tools-focal-java11-ngrams-ARM-cdk-base/bakes/1 (Typerighter needs a large ngrams file to more complicated checks, so we bake it into the AMI rather than download when a box comes up.)

@Freddie Preece @.***>, happy to write that PR while I'm at it, or if you fancy it let me know 👍

On Tue, 8 Aug 2023 at 09:13, Roberto Tyley @.***> wrote:

Uh oh! An unfortunate unanticipated consequence of this change is that the sbt initialize setting added with this PR (checking the Java version https://github.com/guardian/typerighter/blob/677cc03e21f58263c7cdd551e69473fadbcae0b1/build.sbt#L105-L111) causes a job 'failure' https://github.com/guardian/scala-steward-public-repos/actions/runs/5765604057/job/15631890372#step:5:3590 for our Scala Steward process https://github.com/guardian/scala-steward-public-repos/actions/workflows/public-repos-scala-steward.yml

  • which is running with Java 11:

java.lang.AssertionError: assertion failed: Java 8 is required for this project. You are using Java 11.

[image: image] https://user-images.githubusercontent.com/52038/258600045-0a7abbf3-f042-4ab2-a5a4-6024b6879702.png

The error doesn't actually kill the Scala Steward process (other repos get updated successfully), but it does cause the entire job to be reported as a 'failure' (which is a bit noisy), and it's still ongoing (ie the last 3 weeks!).

Even worse, because the job 'fails' I think the GitHub Action is not caching, which has caused the execution time to gradually increase from 3 mins to 25 mins, and Scala Steward to forget when it last opened PRs for dependencies, leading to it offering more frequent updates than it should.

Some possible fixes:

Could you take a look at this @Fweddi https://github.com/Fweddi ?

— Reply to this email directly, view it on GitHub https://github.com/guardian/typerighter/pull/384#issuecomment-1669131279, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3IMF2ZVC6B7LOOFNUQEMTXUHYKLANCNFSM6AAAAAA2KSBOXQ . You are receiving this because your review was requested.Message ID: @.***>

--

Jonathon Herbert · he/him

Senior Developer, Guardian News and Media

@. @.>

Kings Place, 90 York Way,

London N1 9GU

theguardian.com

Download the Guardian app for Android https://play.google.com/store/apps/details?id=com.guardian&hl=en_GB and iOS​ https://itunes.apple.com/gb/app/the-guardian/id409128287?mt=8

--

This e-mail and all attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender and delete the e-mail and all attachments immediately. Do not disclose the contents to another person. You may not use the information for any purpose, or store, or copy, it in any way.  Guardian News & Media Limited is not liable for any computer viruses or other material transmitted with or as part of this e-mail. You should employ virus checking software.   Guardian News & Media Limited is a member of Guardian Media Group plc. Registered Office: PO Box 68164, Kings Place, 90 York Way, London, N1P 2AP.  Registered in England Number 908396

jonathonherbert commented 1 year ago

That's now up as https://github.com/guardian/typerighter/pull/401.

On Tue, 8 Aug 2023 at 09:37, Jonathon Herbert < @.***> wrote:

Thanks for this, Roberto! I've kicked off a bake for a Java 11 version of our ngrams-ready AMI here https://amigo.gutools.co.uk/recipes/editorial-tools-focal-java11-ngrams-ARM-cdk-base/bakes/1 (Typerighter needs a large ngrams file to more complicated checks, so we bake it into the AMI rather than download when a box comes up.)

@Freddie Preece @.***>, happy to write that PR while I'm at it, or if you fancy it let me know 👍

On Tue, 8 Aug 2023 at 09:13, Roberto Tyley @.***> wrote:

Uh oh! An unfortunate unanticipated consequence of this change is that the sbt initialize setting added with this PR (checking the Java version https://github.com/guardian/typerighter/blob/677cc03e21f58263c7cdd551e69473fadbcae0b1/build.sbt#L105-L111) causes a job 'failure' https://github.com/guardian/scala-steward-public-repos/actions/runs/5765604057/job/15631890372#step:5:3590 for our Scala Steward process https://github.com/guardian/scala-steward-public-repos/actions/workflows/public-repos-scala-steward.yml

  • which is running with Java 11:

java.lang.AssertionError: assertion failed: Java 8 is required for this project. You are using Java 11.

[image: image] https://user-images.githubusercontent.com/52038/258600045-0a7abbf3-f042-4ab2-a5a4-6024b6879702.png

The error doesn't actually kill the Scala Steward process (other repos get updated successfully), but it does cause the entire job to be reported as a 'failure' (which is a bit noisy), and it's still ongoing (ie the last 3 weeks!).

Even worse, because the job 'fails' I think the GitHub Action is not caching, which has caused the execution time to gradually increase from 3 mins to 25 mins, and Scala Steward to forget when it last opened PRs for dependencies, leading to it offering more frequent updates than it should.

Some possible fixes:

Could you take a look at this @Fweddi https://github.com/Fweddi ?

— Reply to this email directly, view it on GitHub https://github.com/guardian/typerighter/pull/384#issuecomment-1669131279, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3IMF2ZVC6B7LOOFNUQEMTXUHYKLANCNFSM6AAAAAA2KSBOXQ . You are receiving this because your review was requested.Message ID: @.***>

--

Jonathon Herbert · he/him

Senior Developer, Guardian News and Media

@. @.>

Kings Place, 90 York Way,

London N1 9GU

theguardian.com

Download the Guardian app for Android https://play.google.com/store/apps/details?id=com.guardian&hl=en_GB and iOS​ https://itunes.apple.com/gb/app/the-guardian/id409128287?mt=8

--

Jonathon Herbert · he/him

Senior Developer, Guardian News and Media

@. @.>

Kings Place, 90 York Way,

London N1 9GU

theguardian.com

Download the Guardian app for Android https://play.google.com/store/apps/details?id=com.guardian&hl=en_GB and iOS​ https://itunes.apple.com/gb/app/the-guardian/id409128287?mt=8

--

This e-mail and all attachments are confidential and may also be privileged. If you are not the named recipient, please notify the sender and delete the e-mail and all attachments immediately. Do not disclose the contents to another person. You may not use the information for any purpose, or store, or copy, it in any way.  Guardian News & Media Limited is not liable for any computer viruses or other material transmitted with or as part of this e-mail. You should employ virus checking software.   Guardian News & Media Limited is a member of Guardian Media Group plc. Registered Office: PO Box 68164, Kings Place, 90 York Way, London, N1P 2AP.  Registered in England Number 908396