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

node: Add lv_font_conv to node package #1764

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

FintasticMan
Copy link
Member

This means that these can be installed by simply running npm i.

@github-actions
Copy link

github-actions bot commented May 19, 2023

Build size and comparison to main:

Section Size Difference
text 376784B -16B
data 940B 0B
bss 63420B 0B

@FintasticMan FintasticMan added the maintenance Background work label May 21, 2023
@FintasticMan FintasticMan requested a review from a team May 21, 2023 10:28
@NeroBurner
Copy link
Contributor

If we just need to do npm install, then we could simplify the workflow a bit. Could you update those files in .github as well?

@FintasticMan FintasticMan force-pushed the npm_package branch 6 times, most recently from b137ca2 to db2120c Compare June 22, 2023 16:07
@FintasticMan
Copy link
Member Author

If we just need to do npm install, then we could simplify the workflow a bit. Could you update those files in .github as well?

I've just done this, by installing the packages in the InfiniTime directory, and telling InfiniSim to also search there for the binaries.

I've also updated the docker image to install the npm packages in the InfiniTime directory.

@@ -33,6 +33,8 @@ main() {
[ ! -d "$TOOLS_DIR/$NRF_SDK_VER" ] && GetNrfSdk
[ ! -d "$TOOLS_DIR/mcuboot" ] && GetMcuBoot

npm i
Copy link
Contributor

Choose a reason for hiding this comment

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

why move the install from the docker file where it ran once to the build script where it has to run each build.

please try to move it back to the dockerfile

Copy link
Member Author

Choose a reason for hiding this comment

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

To do that, it would need to clone the InfiniTime repo during the Docker image build. This doesn't really make sense, and I think that it doesn't make sense to need to rebuild the Docker image just to use updated npm packages. It also shouldn't affect the build time much, because if the packages are up to date it doesn't do anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it would require me to have a working internet connection for each build. Previously that was a requirement for just the docker image creation

I think you could use a COPY packages.json command and delete it after the npm command again

Good point. In this case, npm actually runs even if there's no internet connection as long as the packages are up-to-date with the package-lock.json.

You're right, I could just download the package.json and package-lock.json from the repo and use those in the Docker image itself. I personally think that installing the modules in the repo and running npm i for every build is a better solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also prefer to have all dependencies installed during the build of the Docker image rather than during the build of the project. This ensures that once the Docker image is generated and tested, it will be able to build the project no matter what.

Copy link
Member Author

Choose a reason for hiding this comment

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

After looking into it, npm doesn't support installing packages globally from a package.json. If we do want the Docker container to contain the npm packages, we'll have to install them like we do now, without a package.json.

@NeroBurner
Copy link
Contributor

NeroBurner commented Jun 22, 2023 via email

This means that these can be installed by simply running `npm i`.
Also add node_modules to gitignore.
@mark9064
Copy link
Member

mark9064 commented Nov 9, 2023

Might want to remove image conv from the title of the PR to clarify that it's no longer used, I was a bit confused at first

@FintasticMan FintasticMan changed the title node: Add lv_font_conv and lv_img_conv to node package node: Add lv_font_conv to node package Nov 10, 2023
@FintasticMan
Copy link
Member Author

Oh yeah, forgot to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Background work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants