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

Parallel cache push strawman #4467

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

acmorrow
Copy link
Contributor

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

@acmorrow
Copy link
Contributor Author

@bdbaddog - This seems to pass all tests, but please let me know if you would prefer to do this under a --experimental=??? flag and I can rework it like that.

@bdbaddog
Copy link
Contributor

bdbaddog commented Feb 4, 2024

One concern/question.
Previously the node had .cached which you could use to see if the file was cached or not.
Now there's only a check to see if it shouldn't be cached, but no way to tell if the node has actually been cached?

Would restoring .cached break anything in this change?

@acmorrow
Copy link
Contributor Author

acmorrow commented Feb 4, 2024

One concern/question. Previously the node had .cached which you could use to see if the file was cached or not. Now there's only a check to see if it shouldn't be cached, but no way to tell if the node has actually been cached?

Would restoring .cached break anything in this change?

I don't think it would break anything to restore it. I took it out mostly because as far as I could see it was only being used to transfer info about caching from execute to executed, and we don't do that anymore. And I think the test that examined it will still need to go away. So it really will be more or less unused. Fine with me either way though.

@bdbaddog
Copy link
Contributor

bdbaddog commented Feb 5, 2024

One concern/question. Previously the node had .cached which you could use to see if the file was cached or not. Now there's only a check to see if it shouldn't be cached, but no way to tell if the node has actually been cached?
Would restoring .cached break anything in this change?

I don't think it would break anything to restore it. I took it out mostly because as far as I could see it was only being used to transfer info about caching from execute to executed, and we don't do that anymore. And I think the test that examined it will still need to go away. So it really will be more or less unused. Fine with me either way though.

I think it's worth adding back in.
While not part of the published API, I can see someone useing it, and we may find it useful going forward with remote caching,etc..

@acmorrow acmorrow force-pushed the parallel-cache-push-strawman branch from 4accdb3 to e0272d2 Compare February 5, 2024 18:14
@acmorrow
Copy link
Contributor Author

acmorrow commented Feb 5, 2024

One concern/question. Previously the node had .cached which you could use to see if the file was cached or not. Now there's only a check to see if it shouldn't be cached, but no way to tell if the node has actually been cached?
Would restoring .cached break anything in this change?

I don't think it would break anything to restore it. I took it out mostly because as far as I could see it was only being used to transfer info about caching from execute to executed, and we don't do that anymore. And I think the test that examined it will still need to go away. So it really will be more or less unused. Fine with me either way though.

I think it's worth adding back in. While not part of the published API, I can see someone useing it, and we may find it useful going forward with remote caching,etc..

@bdbaddog - done.

@bdbaddog bdbaddog merged commit 14a5f6f into SCons:master Feb 6, 2024
5 of 6 checks passed
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