feat: implement binary prefix detection behavior in packaging #1658
Conversation
|
The idea is good but we cannot easily change the schema at this point, unfortunately. It would destroy backward compatibility, sadly. For now, IMO the only thing we can do is add a CLI flag, and later create a v2 schema where we allow the new value(s). Or we could add a third value (true/false/error) but even that is a "breaking" schema change. |
Gotcha! I will revert the changes on option type then |
|
I think i fixed the bugs now that i spent 3 hours to get into pop os :D (lost my usb stick while moving) and i also manage to apply However 2 runners are failing because of chocolatey extract action, i don't think that should be in our repos but i think its more related to chocolatey @wolfv |
|
I think there is some misunderstanding going on. What we want to introduce is a sanity check to error the package build if any files contain a binary prefix. This should be implemented as an additional CLI flag. Actually, I think we can just do these checks in a post-processing stage and then let the package fail if the flag is activated. |
I am switching to windows ATM and can only reply on mobile but yeah I might have misunderstood the issue, I will try to include the fix for the symlink issue as well today or tomorrow (I will let you know about it too) and will let you know about the progress |
test/end-to-end/test_simple.py
Outdated
| @@ -605,7 +700,7 @@ def check_path(p, t): | |||
| elif path == "force_text/file_without_prefix": | |||
| check_path(p, None) | |||
| elif path == "force_binary/file_with_prefix": | |||
| check_path(p, "binary") | |||
zelosleone
force-pushed
the
erroringhostprefix
branch
from
207fbb6 to
5845966
Compare
|
Looks good and clean :) |
But i gotta fix the windows issue now, I will work on this today xD |
|
@wolfv There were too many problems with symlink tests on windows due to symlinks being broken on creation for packaging (only on windows) (both with windows native commands and python commands, i also activated developer mode and edited the key on regedit to no end) so i switched the symlink test to linux for now. |
src/packaging/file_mapper.rs
Outdated
| } | ||
| #[cfg(windows)] | ||
| { | ||
| let res = if metadata.file_type().is_dir() { |
There was a problem hiding this comment.
the metadata is the symlink_metadata - is that correct, and will it ever evaluate is_dir() == true?
There was a problem hiding this comment.
just checked and i think we can use symlink_dir as well alongside with symlink_metadata to check if first if the target is a directory or not, will add this too now
closes #1507 #1681
I’ve tried to change the usage of bool types to Option Type as well, just because it looked cleaner. I think it is a little bit better for our usage case as well since we will now default to error. I also used proper types instead of bool types for our usage case, just in case we will introduce more types in the future. On windows, binaries aren’t a problem, so I limited the test to UNIX.