search

aws / aws-fpga

Official repository of the AWS EC2 FPGA Hardware and Software Development Kit
Other
1.51k stars 516 forks source link

Remove git clean invocation from DRAM model generation #580

Closed davidbiancolin closed 1 year ago

davidbiancolin commented 2 years ago

Issue #, if available: N/A, though you can find related discussion in firesim/aws-fpga-firesim#59

Description of changes:

Calling git clean to purge the DRAM model build directory is an easy and natural thing to do, but it makes it hard to relocate copies of aws-fpga to remote build instances in cases when aws-fpga may be a git submodule of a super project (since aws-fpga's .git/ will refer to the super project's .git/ under modern versions). This is the case in our project (https://github.com/firesim/firesim).

I was hoping that instead of doing some ourselves, if this could be changed here. I think all that's necessary is to remove tmp/ to cover cases where the build failed previously, but i added all the files the existing git clean would remove for consistency. Let me know if this change would be welcome, and if so, how'd you like it to be modified / further tested.

davidbiancolin commented 2 years ago

Hey, any word on this?

kyyalama2 commented 2 years ago

Hi David,

We are reviewing this change internally and will also kick off some regressions today to make sure the change does not break anything and should be able to get back to you in the next few days. Thank you so much for the follow up

Thanks

davidbiancolin commented 2 years ago

Just a friendly bump on this.

kyyalama2 commented 2 years ago

Hi David, the changes by themselves look good; but we see some timing failures in test_dcp_recipes and other seemingly unrelated failures in Jenkins with this PR. so re-running the jenkins without your changes with a different PR to confirm if its the jenkins setup vs the PR. should know this shortly in the next day. Thanks

davidbiancolin commented 2 years ago

Wonderful, thanks!

davidbiancolin commented 2 years ago

Hey @kyyalama2, could this get into the next release (REL_2022_1) ?