Skip to content

Conversation

@Kriechi
Copy link
Contributor

@Kriechi Kriechi commented Mar 30, 2023

The current build system is using snowpack, which is deprecated and has not received updates in the last two years.
Currently, the repository contains a compiled/generated copy of all resources which is outdated and can cause version mismatches due to race conditions during compilation with snowpack (see FredKSchott/snowpack#3289). This introduces a non-deterministic deployment to be pushed out, potentially containing old (insecure) code. Furthermore, developing locally causes a constant diff against these files.

This PR removes the stale copy of compiled resources from the repository.

This PR migrates from the deprecated snowpack to the actively maintained vite build system, as recommended by snowpack themselves as possible upgrade path (see https://github.com/FredKSchott/snowpack/blob/main/README.md).

This PR restructures the assets folders to be top-level for each resource type, which fits better with vite.

This PR brings in all 3rd party dependencies and libraries to be part of the compiled single page app. No more online downloads from CDNs.

This PR also updates the necessary GitHub Actions workflow files - these need additional testing which I cannot run as part of this PR alone.

Copy link
Collaborator

@makermelissa makermelissa left a comment

Choose a reason for hiding this comment

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

Looks like this isn't passing the checks because it can't find the files. If you can get this working and the site functions without errors, I'm good with these changes.

@Kriechi
Copy link
Contributor Author

Kriechi commented Mar 30, 2023

This is a super annoying bug in snowpack:
FredKSchott/snowpack#3289

I thought I fixed it, as it worked locally, but it seems the timing is just different enough in GitHub Actions to be broken again. I actually believe is is generally broken on the main branch as well, as there is no guarantee that the build+deploy is actually from the expected version of files, but it might as well just use the old version still in the repo. Not deterministic in my understanding.

I spent hours trying to fix this today - but at this point I'm wondering if a different tool chain would be more appropriate?

Update (April 20, 2022): Snowpack is no longer actively maintained and is not recommended for new projects.

@makermelissa
Copy link
Collaborator

@tannewt, since you originally chose snowpack, what are your thoughts?

@Kriechi Kriechi force-pushed the remove-commited-compiled-assets branch from 2d9c598 to 24bafaf Compare April 1, 2023 08:09
@Kriechi Kriechi changed the title remove auto-generated files from repository overhaul build system Apr 1, 2023
@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 1, 2023

@makermelissa @tannewt I updated the PR and migrated from snowpack to vite. Please read through the main PR description for all new changes in this PR.

@makermelissa
Copy link
Collaborator

I did a little more looking into this yesterday and I think this was a good choice of build system to go with. I would just like to get Scott's thoughts first, and I'm not sure his ETA for being back. Also, I'm going to be out most of this next week, so it may not be a good idea to merge right before that in the case of something unexpectedly breaking that needs fixing right away. So because this is a major change and the above reasons, I may hold out on merging until next week.

@tannewt
Copy link
Member

tannewt commented Apr 3, 2023

I don't have any affinity for snowpack or experience with JS. So I'd leave it up to you both.

@makermelissa
Copy link
Collaborator

Ok cool. Works for me. There's a few merge conflicts that need to be fixed first, then I'd like to test this locally, and then I think this will be good to go.

@Kriechi Kriechi force-pushed the remove-commited-compiled-assets branch from 9ad3924 to 0df1fce Compare April 3, 2023 19:03
@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 3, 2023

rebased & fixed conflicts.

Please test extensively, I only know and did the basics using USB, but I did not test the other workflows at all or in depth.
Would be nice to have some automated tests in the future!

@Kriechi Kriechi force-pushed the remove-commited-compiled-assets branch from 0df1fce to a4b7e02 Compare April 11, 2023 20:40
@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 11, 2023

updated and rebased to recent main branch.

@thess
Copy link

thess commented Apr 12, 2023

I don't think this works with the current web-workflow. CP devices directly load device.js via the embedded code.html page.
https://github.com/adafruit/circuitpython/blob/main/supervisor/shared/web_workflow/static/code.html

There must be a way to expose this script using vite (of which I know almost nothing).
Also, you might want to make smaller images to avoid the build warning:
(!) Some chunks are larger than 500 kBs after minification.

Adding this stanza (found on the 'net) seems to work ok.

    build: {
        rollupOptions: {
            output:{
                manualChunks(id) {
                    if (id.includes('node_modules')) {
                        return id.toString().split('node_modules/')[1].split('/')[0].toString();
                    }
                }
            }
        }
    }

That said, I did successfully build and deploy your changes and web-workflow works if invoked directly but not via the CP device landing page (404 error on device.js)

@makermelissa
Copy link
Collaborator

Good catch @thess. That one isn't very obvious.

@Kriechi Kriechi force-pushed the remove-commited-compiled-assets branch from a4b7e02 to 188fa81 Compare April 12, 2023 20:04
@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 12, 2023

@thess would this generated folder structure work for the web workflow?

dist/
├── assets/
│   ├── fa-brands-400-20c4a58b.ttf
│   ├── fa-brands-400-74833209.woff2
│   ├── fa-regular-400-528d022d.ttf
│   ├── fa-regular-400-8e7e5ea1.woff2
│   ├── fa-solid-900-67a65763.ttf
│   ├── fa-solid-900-7152a693.woff2
│   ├── fa-v4compatibility-0515a423.ttf
│   ├── fa-v4compatibility-694a17c3.woff2
│   ├── favicon-badb8633.ico
│   ├── index-1ac6c131.css
│   ├── index-b1ab6ff1.js
│   ├── js/
│   │   └── device.js
│   ├── loading-blinka-0bd52a91.gif
│   ├── loading-blinka-d0565150.webp
│   ├── logo@2x-78412641.png
│   └── logo@3x-c5751f91.png
└── index.html

@makermelissa
Copy link
Collaborator

I believe so. The device.js file must be accessible at https://code.circuitpython.org/assets/js/device.js. The purpose of this script is to bootstrap load the website via javascript as if it is loading it right on the device. You can test by loading the site locally as secure and going to https://localhost:8080/code/#host=circuitpython.local with a device plugged in. Change circuitpython.local as needed for the device host name.

@makermelissa
Copy link
Collaborator

Actually, it looks like I only have the code/index.html on my computer locally. Here's the contents:

<!doctype html>
<html lang="en">
    <head>
        <meta charset="utf-8">
        <meta name="viewport" content="width=device-width, initial-scale=1">
        <link rel="icon" href="https://code.circuitpython.org/assets/images/favicon.ico" type="image/x-icon" />
        <title>Online Code Editor</title>
    </head>

    <body>
        <script type="module" src="http://localhost:8080/assets/js/device.js"></script>
        <p>Uh oh! It looks like you may be offline. You can go to the <a href="/fs/">file browser</a> if you'd like to work with files without connecting to the internet.</p>
    </body>
</html>

@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 12, 2023

Thanks - then this should work just fine. I already updated the PR to generate exactly the folder structure as shown above. I simple moved the device.js file into the special-purpose public/ folder for vite to copy it as-is into the output dist/ folder with the correct intermediary sub-folder structure.

@thess
Copy link

thess commented Apr 12, 2023

@Kriechi - Looks good, thanks for this
@makermelissa - thanks for the debug hints.

@thess
Copy link

thess commented Apr 13, 2023

One more item -- favicon.ico needs to be exposed in assets/images/favicon.ico for the same reason as device.js was (referenced by static/embeddded code.html)

@Kriechi Kriechi force-pushed the remove-commited-compiled-assets branch from 188fa81 to 8db82a9 Compare April 13, 2023 20:33
@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 13, 2023

Done!

@thess
Copy link

thess commented Apr 13, 2023

Sorry for being such a nag -- I'm not very well versed in this environment.

I noticed the logo is not rendering in the vite production build when served by a regular web server like nginx. The embedded base64 PNG is causing 404 errors. Moving the logo??.png files to the public folder and fixing up index.html to find them seems to solve this issue.

I hope this is the last tweak on this PR.

@Kriechi Kriechi force-pushed the remove-commited-compiled-assets branch from 8db82a9 to a380211 Compare April 13, 2023 21:16
@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 13, 2023

Fixed!

New cleaned up folder structure:

dist
├── assets
│   ├── fonts
│   │   ├── fa-brands-400.ttf
│   │   ├── fa-brands-400.woff2
│   │   ├── fa-regular-400.ttf
│   │   ├── fa-regular-400.woff2
│   │   ├── fa-solid-900.ttf
│   │   ├── fa-solid-900.woff2
│   │   ├── fa-v4compatibility.ttf
│   │   └── fa-v4compatibility.woff2
│   ├── images
│   │   ├── favicon.ico
│   │   ├── loading-blinka.gif
│   │   ├── loading-blinka.webp
│   │   ├── logo.png
│   │   ├── logo@2x.png
│   │   └── logo@3x.png
│   ├── index.css
│   └── js
│       ├── device.js
│       └── index.js
└── index.html

@Kriechi Kriechi force-pushed the remove-commited-compiled-assets branch from a380211 to d9b7dde Compare April 13, 2023 21:19
@makermelissa
Copy link
Collaborator

This looks really good. The only change that I would like to see is an option to be able to run this as https by passing a command. This was something I used frequently prior to this PR and this PR appears to remove that functionality.

As it is, chrome doesn't accept the SSL cert just by passing --https as an option to vite. An alternate config that allows use of generated self signs certificates would probably be easiest.

@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 17, 2023

Curious what use cases you have for HTTPS specifically?
At least the specific web-serial APIs are fully functional when using "localhost".

Without having used or reviewed them myself, these seem to be popular plugins for vite:
https://github.com/vitejs/vite-plugin-basic-ssl
and
https://github.com/liuweiGL/vite-plugin-mkcert

If one of these fits your needs and previous workflow, I can link them in the README (again).

@makermelissa
Copy link
Collaborator

Mostly because the final site is https://code.circuitpython.org and it's useful to test because sometimes CORS issues come up where they may not on http or vice versa. It's also sometimes useful to test it as http, which is why I'd like the option.

The mkcert option sounds good. Thanks.

@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 17, 2023

@makermelissa you can probably keep that in a local (not committed to the repo) config file, and pass it to vite using a command like npx vite --config secure-config.js dev
https://vitejs.dev/config/

Or do you want to make it part of the default config? Not sure how many actually need it.

@makermelissa
Copy link
Collaborator

@makermelissa you can probably keep that in a local (not committed to the repo) config file, and pass it to vite using a command like npx vite --config secure-config.js dev https://vitejs.dev/config/

Or do you want to make it part of the default config? Not sure how many actually need it.

This sounds like a good solution.

Copy link
Collaborator

@makermelissa makermelissa left a comment

Choose a reason for hiding this comment

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

Please fix the merge conflicts and then I'm happy to merge this.

@Kriechi Kriechi force-pushed the remove-commited-compiled-assets branch from d9b7dde to 7dc0212 Compare May 2, 2023 05:59
@Kriechi
Copy link
Contributor Author

Kriechi commented May 2, 2023

rebased & no more conflicts!

@makermelissa makermelissa merged commit ce38944 into circuitpython:main May 2, 2023
@makermelissa
Copy link
Collaborator

Thank you for your hard work on this.

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