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

Update NPM installation for containers #1849

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

blm768
Copy link
Contributor

@blm768 blm768 commented Sep 3, 2023

This resolves a deprecation warning with the nodejs package (#1846).

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Build checks have not completed. Possible reasons for this are:

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed

@FintasticMan
Copy link
Member

The docker container doesn't build for some reason, could you look into that? Also, I think that it would be better to only update the main docker image as we don't want to have to maintain the devcontainer separately, and this doesn't fix all the issues with it.

@blm768
Copy link
Contributor Author

blm768 commented Sep 4, 2023

Hmm; might be some caching-related thing. It does build on my machine, although I'm using Podman rather than Docker. I'll see what I can get figured out.

@blm768
Copy link
Contributor Author

blm768 commented Sep 5, 2023

Looks like the package caching was causing non-deterministic build failures of some kind. Not sure why, but it's not a necessary component.

@FintasticMan
Copy link
Member

Yeah, the caching doesn't seem that important to me, because it only makes a difference when rebuilding the docker image, which isn't necessary very often. It was also out of scope for this PR.

@blm768
Copy link
Contributor Author

blm768 commented Sep 5, 2023

Agreed; it mostly just helps when actively tinkering with the Dockerfile, and for that, it's not hard to just temporarily add caching on one's own machine, so it's probably best to leave that to the local user.

@JF002
Copy link
Collaborator

JF002 commented Sep 9, 2023

If I understand correctly, we need to apply this change because the old way to install nodejs is now deprecated?

Also, now, the container seems to build on the CI. Have you check that the Docker container works as expected?

@blm768
Copy link
Contributor Author

blm768 commented Sep 12, 2023

If I understand correctly, we need to apply this change because the old way to install nodejs is now deprecated?

Yep; this is a fix for #1846.

Also, now, the container seems to build on the CI. Have you check that the Docker container works as expected?

I'm able to start building, but something's off with swc:

[ 36%] Built target lvgl
/usr/lib/node_modules/ts-node/src/transpilers/swc.ts:262
      throw new Error(
            ^
Error: @swc/core threw an error when attempting to validate swc compiler options.
You may be using an old version of swc which does not support the options used by ts-node.
Try upgrading to the latest version of swc.
Error message from swc:
Failed to deserialize buffer as swc::config::Options
JSON: {"sourceMaps":true,"module":{"noInterop":false,"type":"commonjs","strictMode":true,"ignoreDynamic":false},"swcrc":false,"jsc":{"parser":{"syntax":"typescript","tsx":false,"dynamicImport":true,"importAssertions":true},"target":"es5","transform":{"legacyDecorator":true,"react":{"throwIfNamespace":false,"useBuiltins":false,"runtime":"automatic"}},"keepClassNames":false,"experimental":{"keepImportAssertions":true}}}

Caused by:
    unknown field `keepImportAssertions`, expected one of `plugins`, `keepImportAttributes`, `emitAssertForImportAttributes`, `cacheRoot`, `disableBuiltinTransformsForInternalTesting` at line 1 column 415
    at createVariant (/usr/lib/node_modules/ts-node/src/transpilers/swc.ts:262:13)
    at createSwcOptions (/usr/lib/node_modules/ts-node/src/transpilers/swc.ts:211:25)
    at create (/usr/lib/node_modules/ts-node/src/transpilers/swc.ts:56:41)
    at createTranspiler (/usr/lib/node_modules/ts-node/src/index.ts:784:16)
    at createTranspileOnlyGetOutputFunction (/usr/lib/node_modules/ts-node/src/index.ts:1341:28)
    at createFromPreloadedConfig (/usr/lib/node_modules/ts-node/src/index.ts:1404:34)
    at phase4 (/usr/lib/node_modules/ts-node/src/bin.ts:543:44)
    at bootstrap (/usr/lib/node_modules/ts-node/src/bin.ts:95:10)
    at main (/usr/lib/node_modules/ts-node/src/bin.ts:55:10)
    at Object.<anonymous> (/usr/lib/node_modules/ts-node/src/bin.ts:800:3)
Traceback (most recent call last):
  File "/sources/src/resources/generate-img.py", line 56, in <module>
    main()
  File "/sources/src/resources/generate-img.py", line 51, in main
    subprocess.check_call(line)
  File "/usr/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/usr/bin/lv_img_conv', '/sources/src/resources/images/pine_logo.png', '--force', '--output-file', 'pine_small.bin', '--color-format', 'CF_TRUE_COLOR_ALPHA', '--output-format', 'bin', '--binary-format', 'ARGB8565_RBSWAP']' returned non-zero exit status 1.

That seems to be broken even if I build using the old container, though.

@JosuGZ
Copy link

JosuGZ commented Sep 28, 2023

Why there is no package.json and package-lock.json? I'm facing similar issues and updating one library seems to break another.

Copy link
Member

@FintasticMan FintasticMan left a comment

Choose a reason for hiding this comment

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

Looks good to me. The issue mentioned in the comments has been resolved already.

@FintasticMan FintasticMan merged commit 9b8eb75 into InfiniTimeOrg:main Nov 12, 2023
6 checks passed
@JF002 JF002 added this to the 1.14.0 milestone Dec 10, 2023
@JF002 JF002 mentioned this pull request Dec 10, 2023
1 task
@blm768 blm768 deleted the update-containers branch December 10, 2023 17:56
mark9064 pushed a commit to mark9064/InfiniTime that referenced this pull request Jan 25, 2024
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.

4 participants