search

AdaCore / e3-core

Core framework for developing portable automated build systems
27 stars 36 forks source link

e3.cve: Close NVD cache session #682

Closed leocardao closed 8 months ago

leocardao commented 8 months ago

When it's not done, this can cause errors on Windows. Windows does not agree to delete a file or directory that is currently open.

grouigrokon commented 8 months ago

I do not see the rationale behind that. Any hint on why we should do that ? Your changes simply lose the point of caching the session.

grouigrokon commented 8 months ago

I know it makes a bit more work, and more reviews ;) but: I would keep the old behavior (with the potential bug) still possible. With these modifications, there is a backward compatibility issue.

What is your advice with that ?

leocardao commented 8 months ago

I know it makes a bit more work, and more reviews ;) but: I would keep the old behavior (with the potential bug) still possible. With these modifications, there is a backward compatibility issue.

What is your advice with that ?

I don't really have any advice at this level, it's pretty trivial to do. In the end, I'd be in favor of adding a method close to ensure consistency in the class and a warning to notify the dev.

In fact, it's always possible to get the bug by calling the __enter__ method explicitly (without using the with keyword) without calling the __exit__ method. But I don't think that's what you're looking for :)

I suggest implementing the session property, which will open the session if it hasn't already been done, but with a DeprecationWarning if it has, to notify the dev that this is not desired (and that one day we may remove this feature). For me, this is the best way to go.

Then we can always use log.warning, but from my point of view, we shouldn't use logs for that.

Does that make sense to you, @grouigrokon?

grouigrokon commented 8 months ago

I don't really have any advice at this level, it's pretty trivial to do. In the end, I'd be in favor of adding a method close to ensure consistency in the class and a warning to notify the dev.

In fact, it's always possible to get the bug by calling the __enter__ method explicitly (without using the with keyword) without calling the __exit__ method. But I don't think that's what you're looking for :)

I suggest implementing the session property, which will open the session if it hasn't already been done, but with a DeprecationWarning if it has, to notify the dev that this is not desired (and that one day we may remove this feature). For me, this is the best way to go.

Then we can always use log.warning, but from my point of view, we shouldn't use logs for that.

Does that make sense to you, @grouigrokon?

That would be ok to me, and I would use warnings ;) There are warnings for the use of the NVD API key ... so why not on that too ?