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

DRAFT: Add gh action to deploy to pages and run test #62

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

Conversation

hardliner66
Copy link
Contributor

Fixes #58

To fix the mime-type problem I had to add:

<link rel="preload" href="/uFork/ufork_wasm.wasm" as="fetch" type="application/wasm" crossorigin="">
<link rel="preload" href="/uFork/ufork_wasm.opt.wasm" as="fetch" type="application/wasm" crossorigin="">

I added two workflows.
Tests: Run on each merge request and when called from another workflow
Pages: Run on each push on the main branch (should include MR merges)

The pages workflow uses the tests workflow to check if the code is valid.

In the future it might be more reasonable to create proper releases and only deploy when a new release is made. That way the web version had some stability guarantees. But that's not a requirement right now.

@jamesdiacono The deno tests return an object with the fields pass, lost and fail. The only field that's filled on my machine is the lost field. If this is a failure, can you make sure to make the test suite return a non-zero exit code? If that's okay, then nothing needs to be done for it to work.

@hardliner66 hardliner66 changed the title Add gh action to deploy to pages and run tests DRAFT: Add gh action to deploy to pages and run test Oct 16, 2023
@hardliner66
Copy link
Contributor Author

I just saw there are problems with loading from lib, which needs fixing. We might need to move the lib folder into www as well for github pages to work.

@dalnefre
Copy link
Contributor

@jamesdiacono could you please look into ways to restructure the deployment directories so all the proper components can be found by the webserver, etc.? I know we have a bunch of node.js and deno stuff that I assume would have similar issues. Also, all the stuff in examples that currently uses lots of .. paths. I'm don't know what would be the least painful way to organize all this. Recommendations requested.

@jamesdiacono
Copy link
Contributor

Hi @hardliner66 , welcome to the uFork team! There's a lot going on in this PR. Perhaps we should get on a call to discuss? Or can we discuss it here?

@jamesdiacono
Copy link
Contributor

Oh I see, the bulk of the changes result from moving the lib directory into www. I'll just leave my comments here.

I have always considered the ufork-wasm directory as the web server root. That way it is possible to access examples, lib, scm, www, and formerly target. The name www is misleading. If it were up to me, www would be named js, and lib would be asm. The debugger, index.html, would be at ufork-wasm/debugger.html. Alternatively, we could make the debugger the homepage by naming it index.html.

Both run_asm_test.sh and test.sh return a non-zero exit on failure, or at least they should. They pass on may machine, so we need to look into why they are failing for you.

There are also some very minor things I'd like to comment on:

  • It appears that your text editor is removing trailing newlines when you save. Dale and my editors are configured to ensure a trailing newline, so that could cause commit churn.
  • I enjoy set -euo pipefail, but it can cause breakage when added to existing scripts. For example, the && chaining in build.sh should be removed. Just as a sidenote, I avoid a blank line between the hashbang and the directive because I see them as one unit controlling the semantics of the rest of the script:
#!/bin/bash
set -euo pipefail

...other commands...
  • I'm not a big fan of BASH_SOURCE. I've had some bad experiences with it in the past, for example when scripts are symlinked. Is it necessary? Our convention so far has been to run scripts from the ufork-wasm directory.

@hardliner66
Copy link
Contributor Author

Yeah, I moved the files in a separate commit so it's easier to undo, if there's a better solution. But I wanted to test it with GitHub pages to see if it works now.

Having the ufork-wasm dir be the root is good for local testing, but I don't think we should just deploy a whole bunch of unnecessary files. I would prefer to split that stuff even more, because right now it's not clear what's needed or not unless you already know the project.

I also prefer BASH_SOURCE everywhere, where the script is location dependent. I understand that the convention works for now, but by using it it will even work for new contributors who don't have that implicit knowledge.

The rest I'll check when I come home, as I'm at my way to work right now. We can plan a meeting if you want as well. What's your timezone?

@jamesdiacono
Copy link
Contributor

I don't really see a problem with deploying the whole of ufork-wasm, as it's all open source and github.io probably doesn't allow visitors to browse directories. But here are the necessary files, assuming we rename things as I proposed:

asm/
scm/
js/
debugger.html

Surely we can specify that in the GitHub action config?

I am GMT+1, but I've been going to bed at about 8pm (and getting up at 4am). What time do you get home from work?

@hardliner66
Copy link
Contributor Author

I'm gmt +1 as well right now (I think) but I have a doctors appointment so I'm not sure when I'll be home. Tomorrow I should be home by 6pm

@dalnefre
Copy link
Contributor

I think a group-call would be a good idea, but let's coordinate via email. I'll send a group message to start.

Originally, I created the www/ directory as the deployed web-root. I think it's a good idea to separate the sources, docs, scripts, etc. from the deployed files. Even though it's open-source, not all the files are part of the deployed application(s). Just like many projects have a bin/ directory for build products. I'm fine with lib->asm renaming. And grouping the files into subdirectories of the web-root is probably good organization for re-use.

BASH_SOURCE sounds like a shell-specific feature, which I'd like to avoid. We want to be a clear and portable as possible. If the same script can run in sh, ksh, zsh, etc. that's ideal. By the same token, if we have a Makefile we should still be conservative in what fancy features we use. Keep it simple. I think scripts should generally be design to run from the directory in which they live. Perhaps the serve.sh script should cd www (or equivalent) to establish the web-root.

The project-family is growing and naturally accumulating complexity. Now is a good time to discuss project structures to support further growth.

@dalnefre
Copy link
Contributor

dalnefre commented Nov 29, 2023

I've merged @jamesdiacono restructuring increment (PR #64), which was based on this PR and our previous discussion. The C-based protoype has been moved into its own repository. The ufork.org domain now points to the github pages deployment. For example, the debugger lives at https://ufork.org/debugger/

What additional changes are required to prepare the uFork core for publication on crates.io? Perhaps I separate issue/PR would be appropriate.

@hardliner66
Copy link
Contributor Author

@dalnefre Sorry for the late response, I'm gonna have a look at the changes and make the appropriate changes and create a separate PR for publishing to crates.io.

@dalnefre
Copy link
Contributor

dalnefre commented Dec 3, 2023

@hardliner66 There's no schedule here. We appreciate your efforts when you have the time to contribute!

I expect that some rustdoc would also be important to make the crate(s) useful. I believe I have the credentials already established to the publish the crate, when the time is right.

@dalnefre
Copy link
Contributor

dalnefre commented Dec 3, 2023

The comment block describing the run_loop method would probably make a good rustdoc example, if you wanted to include that in your PR.

The run-loop is the main entry-point for a host to run the uFork processor.

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.

Add github actions
3 participants