Distinguish unsafe and extern - #55
Conversation
| @@ -1,5 +1,5 @@ | |||
| [common] | |||
| version = 2 | |||
| version = 3 | |||
There was a problem hiding this comment.
To avoid the CI failure, how about leaving our config file at version 2 until at least after the next release? Version 2 should presumably mean that allow_unsafe implies allow_extern.
There was a problem hiding this comment.
I tried that before. The CI nevertheless fails.
There was a problem hiding this comment.
If it fails with version = 2, then we should probably fix that. The whole idea of the versioning is that users can opt in to new behaviour on their own schedule. Why does it fail?
There was a problem hiding this comment.
Because as stated in the description regardless of the version #[serde(deny_unknown_fields)], denies parsing of the new field before the other logic to check the version gets executed. This is definitely not good and I tried to look into it, but I could not find a good solution on how to fix that.
There was a problem hiding this comment.
Are we now reading and tokenising every file twice now? Once for checking for unsafe and once for checking for extern?
There was a problem hiding this comment.
Yeah, that is indeed correct and not ideal. I split it to keep it as separate and easy to understand as possible for now. But it could also be merged, once it has been confirmed that this code in general is correct and useful.
There was a problem hiding this comment.
Oh, right, sorry. I meant completely revert the changes to cackle.toml, so the new fields wouldn't be set
There was a problem hiding this comment.
Ok, I have reverted both now, but that does not help either as then the tests are failing. I don't really understand where you want to go with this.
There was a problem hiding this comment.
From the failure, it looks like the tests are failing because they're saying that the "extern" permission is needed. When I was suggesting reverting the changes to cackle.toml, I meant just the cackle.toml for cackle itself, not the one for the test.
I suspect we also need to make it so that if a config has version=2, then we copy the value from allow_unsafe to allow_extern. That way a version 2 config will still work with a newer version of cackle, which I think is important.
There was a problem hiding this comment.
Ok, I changed it to only revert the main cackle.toml.
I will try to add this fallback for version 2 tomorrow.
There was a problem hiding this comment.
Is the change I made now what you had in mind?
e0ec1cf to
4eeabff
Compare
4eeabff to
6897b9b
Compare
2 participants
Builds on #49, so should be merged afterwards.
This creates a new permission to track
externusage, in particularexternblocks andexternfunctions, but deliberately notextern crates.Packages will likely require
unsafepermission as well.There are no tests yet.
The CI is failing as it unfortunately fails to parse the new config field before checking the version of the config file format.