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

#275: Update copyright year and GHA to check year #282

Merged
merged 18 commits into from
Jun 13, 2024

Conversation

slry
Copy link
Contributor

@slry slry commented May 13, 2024

In this PR I updated the copyright year of MIT license and added Workflow that runs script to check MIT license year on push and pull request
@yegor256, please take a look

Closes #275


PR-Codex overview

This PR adds a GitHub Actions workflow to check for outdated copyright years in files based on the specified pattern.

Detailed summary

  • Added a GitHub Actions workflow for copyright check
  • Checks for outdated copyright years in files
  • Ignores specified files and directories
  • Extracts and compares years to the current year
  • Outputs files with outdated copyright years

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@yegor256
Copy link
Member

@slry looks good, but do we really need a separate .sh file for this? Can't we put everything into the run: block in the YAML file?

@slry
Copy link
Contributor Author

slry commented May 13, 2024

@yegor256 done.

@slry
Copy link
Contributor Author

slry commented May 15, 2024

@yegor256 please, take a look at changes, we may merge it.

@yegor256
Copy link
Member

@maxonfjvipon please, help us review this one

Copy link
Member

Choose a reason for hiding this comment

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

@slry why do we need these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess, my IDE accidentally formatted this file, will revert this one

@maxonfjvipon
Copy link
Member

@slry before actual review we need all GHA checks are green

@slry
Copy link
Contributor Author

slry commented May 28, 2024

@maxonfjvipon I pulled updates from master and it passed markdownlint GHA

But some of the build actions are failing because GHA cannot find Node of version 12 for arm64

Error: Unable to find Node version '12' for platform darwin and architecture arm64.

I do not think this is the problem with this PR

@maxonfjvipon
Copy link
Member

maxonfjvipon commented May 28, 2024

@slry you're right, but we won't be able merge your PR without these checks. You either should try to fix it, or wait until it's resolved by someone else in other PR

@slry
Copy link
Contributor Author

slry commented May 28, 2024

@maxonfjvipon seems like this issue relates to this post

Right now node12 is not supported by MacOS ARM64, using node16 could solve this issue

What do you think?

@maxonfjvipon
Copy link
Member

@slry let's try

@slry
Copy link
Contributor Author

slry commented May 28, 2024

@maxonfjvipon I fixed macos problem, but tests still do not pass

It seems like some of the npm packages do not support node12 and requires >=node16

/home/runner/work/eoc/eoc/node_modules/commander/lib/command.js:380
eOrNameAndArgs = enableOrNameAndArgs ?? 'help [command]';
                                               ^

SyntaxError: Unexpected token '?'
    at wrapSafe (internal/modules/cjs/loader.js:915:16)
    at Module._compile (internal/modules/cjs/loader.js:963:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object.<anonymous> (/home/runner/work/eoc/eoc/node_modules/commander/index.js:2:21)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
Error: Process completed with exit code 1.

UPD: I also checked package.json file with dependecies and I assume that this project uses latest version of node and dependecies, but tests runs only on node12 and node16 which does not make sense

@maxonfjvipon
Copy link
Member

@slry you're right. commander package uses Nullish Coalescing Operator which is supported on Node >= 14.0. Let's try to replace Node 12 with Node 14 in our GHA CI matrix.

@slry
Copy link
Contributor Author

slry commented May 28, 2024

@maxonfjvipon now tests failing on Windows machine, I assume it cannot parse linux commands (maybe removing \ will fix it...)

      - run: |
          cd itest
          p=0.36.0
          t=0.36.0
          node ../src/eoc.js "--parser=$p" "--home-tag=$t" --batch \
            dataize program
          node ../src/eoc.js "--parser=$p" "--home-tag=$t" --alone \
            --batch dataize program
          node ../src/eoc.js clean
          node ../src/eoc.js "--parser=$p" "--home-tag=$t" --batch \
            test

GHA output:

ParserError: D:\a\_temp\ff659728-8805-4818-afa3-684a0463d1c4.ps1:8
Line |
   8 |    --batch dataize program
     |      ~
     | Missing expression after unary operator '--'.
Error: Process completed with exit code 1.

UPD: also I think there is also a problem with using environment variables in Windows

@slry
Copy link
Contributor Author

slry commented May 28, 2024

@maxonfjvipon @yegor256 please approve this changes

node: [12, 16]
node: [14, 16]
exclude:
- os: macos-latest
Copy link
Member

Choose a reason for hiding this comment

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

@slry even Node 14 is not supported on MacOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxonfjvipon I have tested it, unfortunately its not...

- uses: actions/checkout@v4
- name: Run check
run: |
bash - << 'SCRIPT'
Copy link
Member

Choose a reason for hiding this comment

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

@slry I can't understand, why do you need to run Bash script through this SCRIPT file? can't you just directly use the text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yegor256 this script just do not work in yaml for some reason, so this is a workaround I found

Copy link
Member

Choose a reason for hiding this comment

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

@slry it's a weird workaround. I suggest you try to make it work as all other scripts.

@slry
Copy link
Contributor Author

slry commented Jun 7, 2024

@maxonfjvipon @yegor256 can we merge this?

@yegor256
Copy link
Member

yegor256 commented Jun 7, 2024

@slry there are merge conflicts, can't merge

@slry
Copy link
Contributor Author

slry commented Jun 7, 2024

@yegor256 resolved conflicts

@slry
Copy link
Contributor Author

slry commented Jun 9, 2024

@yegor256 made some changes to script:

  1. Removed my workaround bash - << SCRIPT
  2. Fixed some bugs

@slry
Copy link
Contributor Author

slry commented Jun 11, 2024

@yegor256 can we merge this?

1 similar comment
@slry
Copy link
Contributor Author

slry commented Jun 12, 2024

@yegor256 can we merge this?

@slry
Copy link
Contributor Author

slry commented Jun 13, 2024

@yegor256 please take a look at changes

@yegor256
Copy link
Member

@rultor merge

@rultor
Copy link
Contributor

rultor commented Jun 13, 2024

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit a86292e into objectionary:master Jun 13, 2024
30 checks passed
@rultor
Copy link
Contributor

rultor commented Jun 13, 2024

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 30min)

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.

Update license copyright year (Workflow)
4 participants