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

Makefile: target to import all programs from Optimism #2906

Merged
merged 3 commits into from
Jan 2, 2025

Conversation

dannywillems
Copy link
Member

@dannywillems dannywillems commented Dec 23, 2024

Import all MIPS programs from Optimism, and tweak it to make it compatible with our ELF loader. Requested by @martyall to have more small test programs for the o1vm MIPS edition.

@dannywillems dannywillems force-pushed the dw/mips-makefile-import branch 2 times, most recently from 23654a2 to b8a6a11 Compare December 23, 2024 18:21
@dannywillems dannywillems force-pushed the dw/mips-makefile-import branch from b8a6a11 to 49235d3 Compare December 23, 2024 20:43
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.45%. Comparing base (43c4dfa) to head (2637499).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2906      +/-   ##
==========================================
- Coverage   73.45%   73.45%   -0.01%     
==========================================
  Files         259      259              
  Lines       61593    61593              
==========================================
- Hits        45244    45243       -1     
- Misses      16349    16350       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@martyall martyall left a comment

Choose a reason for hiding this comment

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

This is super useful! My only request is that the linked files shouldn't be committed, i.e. the entire resources/programs/{mips,riscv}/bin directory should be ignored (not just the object files). It seems like this was also your intention since make clean blows this directory away.

@dannywillems
Copy link
Member Author

This is super useful! My only request is that the linked files shouldn't be committed, i.e. the entire resources/programs/{mips,riscv}/bin directory should be ignored (not just the object files). It seems like this was also your intention since make clean blows this directory away.

The only issue I see by not committing them is that the CI (and users) will have to build them, and therefore would need to add the mips toolchain on their machine.

@martyall martyall mentioned this pull request Dec 24, 2024
@martyall
Copy link
Contributor

martyall commented Dec 24, 2024

The only issue I see by not committing them is that the CI (and users) will have to build them, and therefore would need to add the mips toolchain on their machine.

That's a fair point that it's annoying to require it for devs, it just seems weird that running make clean creates a diff by deleting them.

BTW I can confirm that these are working with #2911 .

@martyall
Copy link
Contributor

martyall commented Dec 26, 2024

@dannywillems I would suggest that we use a github workflow with a workflow_dispatch trigger to be able to run this job in ci and tar/upload the artifacts. This way we can still ignore the bin directory and make clean is really clean (as opposed to "dirty"). Developers who don't want to build the binary files from source (e.g. mac users) can just download and unzip.

I implemented this strategy in #2911. You can see what this workflow looks like here, and here is an example upload

@dannywillems dannywillems force-pushed the dw/mips-makefile-import branch from 49235d3 to 4cf77b5 Compare January 2, 2025 09:56
@dannywillems dannywillems force-pushed the dw/mips-makefile-import branch from 4cf77b5 to 2637499 Compare January 2, 2025 09:56
@dannywillems
Copy link
Member Author

I did remove the commit 49235d3 to clean the history and avoid unnecessary commits. #2911 would therefore need a rebase.

@dannywillems dannywillems merged commit 23f91ca into master Jan 2, 2025
8 checks passed
@dannywillems dannywillems deleted the dw/mips-makefile-import branch January 2, 2025 17:34
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.

2 participants