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

Clean up all build files #745

Closed
wants to merge 1 commit into from
Closed

Clean up all build files #745

wants to merge 1 commit into from

Conversation

xyb
Copy link
Contributor

@xyb xyb commented Nov 22, 2019

fix #555

@xyb xyb changed the title Clean up all build files WIP: Clean up all build files Nov 23, 2019
@xyb xyb changed the title WIP: Clean up all build files Clean up all build files Nov 23, 2019
@genotrance
Copy link
Contributor

genotrance commented Jan 31, 2020

This is good work, can you please resolve conflicts and resubmit? Thank you.

Edit: Ideally, nimpretty changes could be done separately from the actual code changes so that it is easier to tell the difference. If possible, please squash into two commits - first with nimble clean related additions followed by a nimpretty commit.

Copy link
Collaborator

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Please separate into two PRs (one for nimpretty). Otherwise reviewing this is a pain.

@xyb xyb mentioned this pull request Feb 12, 2020
@xyb
Copy link
Contributor Author

xyb commented Feb 12, 2020

@genotrance @dom96 Sure, I have separated first PR #774.

@xyb
Copy link
Contributor Author

xyb commented Feb 13, 2020

@dom96 @genotrance I removed changes created by nimpretty, now it's included nimble clean related only.

@timotheecour
Copy link
Member

I think this is the wrong approach.

Here are some problems:

  • if nimble test crashes for whatever reason (maybe even Control C), generated files/dirs won't get cleaned up; even on the next run; eg:
    let existsBefore = existsFile(binFileName) would be true on next run. This can be addressed, but at the risk of introducing false positives

  • the algorithm to find files that should be cleaned up is fragile. Right now it just collects tfoo for a file tfoo.nim. This won't work if binaries are named differently, or for generated shared libraries or if tests generate other binary or non binary files, as is even the case with nimble's own nimble test, which generates other non binary files eg .json, .nimble, .nims extensions. This is not unique to nimble's tests, nim's compiler nimble pkg, and others.

  • a tool cleaning up files using a complex logic makes me nervous it could also remove files that should not be removed, which would be even worse. The logic may not be complex right now but it will be if it intends to reduce those false negatives

  • nimble test ideally should keep git status clean (even before nimble clean); this approach doesn't do that

Finally, a use case this prevents is to allow comparing 2 runs of nimble test and compare the output artifacts.

see #787 for what I'm proposing instead.

@xyb xyb closed this Dec 10, 2020
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 a nimble clean command to clean up build files.
4 participants