Skip to content

Conversation

@pipe01
Copy link
Contributor

@pipe01 pipe01 commented Oct 28, 2024

Using nightly.link and corsproxy.io we can generate a link that, when clicked, will direct users to a build of the latest commit of a pull request running on InfiniEmu.

It's also worth noting that GitHub actions artifacts expire after 90 days.

@github-actions
Copy link

github-actions bot commented Oct 28, 2024

Build size and comparison to main:

Section Size Difference
text 373008B 0B
data 948B 0B
bss 22536B 0B

Run in InfiniEmu

@liamcharger
Copy link
Contributor

liamcharger commented Oct 28, 2024

Would really like to see this merged, this makes testing PRs much easier!

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.

Love it!

@mark9064
Copy link
Member

Is using an open CORS proxy wise? It feels dangerous to trust it with user traffic - if it started acting maliciously (got hacked, owner decided to cash in etc) it could return whatever it wanted to user's browsers

@pipe01
Copy link
Contributor Author

pipe01 commented Oct 30, 2024

Maybe not, but the worst that could happen is that they serve either an invalid ZIP file which would fail to load, or they could serve a different firmware file which would just be annoying.

@mark9064
Copy link
Member

I'm trying to think if there's another way to load it, could InfiniEmu itself fetch the firmware somehow?

@pipe01
Copy link
Contributor Author

pipe01 commented Oct 30, 2024

If you mean fetching from GitHub directly it's not really possible because there's no official way to download artifacts without being logged in or using an API key, both of which aren't really feasible in this case. That's what nightly.link solves, however it unfortunately doesn't allow CORS.

@mark9064
Copy link
Member

Hmm, could the InfiniEmu server fetch it and serve the artefact to the user?

@pipe01
Copy link
Contributor Author

pipe01 commented Oct 30, 2024

There's no server per se right now, it's hosted on GitHub pages.

I've made a slight improvement by not hardcoding the CORS proxy into the URL in the comment and instead letting InfiniEmu handle that so that it's easier to change in the future if needed.

@mark9064
Copy link
Member

👍 makes sense
Excuse my ignorance, but would requiring being logged in to github remove the need for the proxy? Since anyone who's going to comment on the PR has to be logged in anyway, I don't think it would be an issue

@pipe01
Copy link
Contributor Author

pipe01 commented Oct 30, 2024

(Un)fortunately you can't do that because that would mean that you could send any requests on behalf of the logged in user from a different webpage, which is obviously not great.

@mark9064
Copy link
Member

Ah, I thought the github cookies for that might be allowed to be samesite, but I guess it makes sense that they're not and there'd have to be a full Oauth flow (which isn't possible on GH pages I guess)

@mark9064 mark9064 added the new feature This thread is about a new feature label Nov 18, 2024
@pipe01
Copy link
Contributor Author

pipe01 commented Jan 28, 2025

@mark9064 I think this is ready to merge 🙏 Would be nice to have soon to test PRs as they come in

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

Still not a fan of using a CORS proxy but that's now up to Infiniemu itself which could be refactored, so I guess that problem is now pushed out of scope

@mark9064 mark9064 merged commit 7b39d81 into InfiniTimeOrg:main Jan 28, 2025
5 checks passed
dariusarnold pushed a commit to dariusarnold/InfiniTime that referenced this pull request Mar 2, 2025
BurninTurtles pushed a commit to BurninTurtles/InfiniTimeBT that referenced this pull request Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature This thread is about a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants