Implement xattr support on Windows #800
Conversation
Sync autopkg/autopkg:master
Update fork 20210502
xattr with pyads change config file location. recipe remove attribute settings
Remove platform warning
|
This seems reasonable, but can you please provide some output from AutoPkg on both macOS and Windows to show that this actually works as intended? I'd like to see what the relevant output is, where the data is stored on Windows, and how it tracks etags. |
| @@ -806,6 +807,9 @@ def repo_delete(argv): | |||
| recipe_search_dirs.remove(repo_path) | |||
| # now remove the repo files | |||
| try: | |||
| if is_windows(): | |||
| # we need to remove readonly, hidden and system bits | |||
| os.system("attrib.exe -r -h -s " + repo_path + "\\*.* /S /D") | |||
There was a problem hiding this comment.
Why os.system here instead of subprocess?
There was a problem hiding this comment.
Just simplicity, i think. It is working with os.system, but could of course use subprocess too.
|
Ok, here is an output from a Windows machine: |
|
Hi Nick, |
Merge 2.7.1 to NickETH/autopkg
|
The step of the test that is failing is the e2e test, running the following:
and it is not finding the |
|
Also, this PR has changes in it that are NOT only "Implement xattr support on Windows" and seems to contain 3 different sets of changes, and a new processor. Instead of adding pyads as a PIP dependency, it is actually putting pyads.py into the project, which seems bad. |
| # -*- coding: utf-8 -*- | ||
| """ | ||
| Copyright © 2015, Robin David - MIT-Licensed | ||
| Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated | ||
| documentation files (the "Software"), to deal in the Software without restriction, including without limitation | ||
| the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and | ||
| to permit persons to whom the Software is furnished to do so, subject to the following conditions: | ||
| The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. | ||
| The Software is provided "as is", without warranty of any kind, express or implied, including but not limited | ||
| to the warranties of merchantability, fitness for a particular purpose and noninfringement. In no event shall | ||
| the authors or copyright holders X be liable for any claim, damages or other liability, whether in an action | ||
| of contract, tort or otherwise, arising from, out of or in connection with the software or the use or other | ||
| dealings in the Software. | ||
| Except as contained in this notice, the name of the Robin David shall not be used in advertising or otherwise | ||
| to promote the sale, use or other dealings in this Software without prior written authorization from the Robin David. | ||
| """ | ||
| import os | ||
| import sys | ||
| from ctypes import * |
There was a problem hiding this comment.
This doesn't belong as a folder/file in the project itself, but rather a pip dependency that is windows only.
There was a problem hiding this comment.
Bad idea...
This module does a complete different thing (it is for PLC API)!
Unfortunateley, they are both named the same.
There is no module for pyads for alternate data streams, i am aware of.
There was a problem hiding this comment.
oh you are right, this is a different pip library. So where is this one you are using from?
| if is_windows(): | ||
| # we need to remove readonly, hidden and system bits | ||
| os.system("attrib.exe -r -h -s " + repo_path + "\\*.* /S /D") |
There was a problem hiding this comment.
This seems to be to fix a bug that is unrelated to fixing xattr. What is the issue this is solving? Is there a pythonic way to solve this using Pathlib or similar, rather than calling attrib.exe?
I use autopkg a ton on windows, and I have never hit the problem that this seems to be fixing. What autopkg command causes this problem? How can this be reproduced?
There was a problem hiding this comment.
This changes came into this PR, after i issued it.
Was not intend to do so...
There was a problem hiding this comment.
ah, I see. That is a pain. Once you create the PR from a branch, if you make any further changes to that branch, then it gets included.
It is best to create a custom branch only for the purposes of the PR, which is a bit annoying, but it is the way to cleanly avoid this, especially since this PR has been open for a very long time.
| result = False | ||
| try: | ||
| # result = eval(predicate_string) | ||
| splitted_ops = predicate_string.split() |
There was a problem hiding this comment.
I think this is assuming that predicates have a space between the left side, the right side, and the operator(s) but I don't think that is always the case. Is download_changed!=True a valid predicate? I don't actually have enough experience with them to know for sure.
Would be good to provide examples of what predicate options are specifically supported on windows. Seems like it is only != and == but only if there is a single space between the operator and the left side / right slide.
There was a problem hiding this comment.
This isn't a true replacement for StopProcessingIf's use of predicates, but it does work the same way cross platform: https://github.com/jgstew/jgstew-recipes/blob/main/SharedProcessors/StopProcessingIfDownloadUnchanged.py
| description = __doc__ | ||
|
|
||
| def main(self): | ||
| if not is_windows(): |
There was a problem hiding this comment.
Can powershell core do Get-AuthenticodeSignature on non-windows? I'm not certain, but if so, then the decision not to run this might have more to do with finding a powershell binary to execute than it is to do with is_windows().
I have never tried to use Get-AuthenticodeSignature in Powershell core on non-windows, but I should try it.
| self.output("Code signature verification disabled for this recipe " "run.") | ||
| return | ||
| input_path = self.env["input_path"] | ||
| powershell = "C:\\windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe" |
There was a problem hiding this comment.
This is a very minor point, not a blocker, but I like the idea of the powershell path being an input variable, but with "C:\\windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe" being the default path.
I like to use a Processor that takes an array of paths and looks for the first one it find that is an executable file and returns it, then passes that as input into the next processor like this one, which makes it easier to implement this using PowerShell core instead of Powershell, or non-windows if that works, which I haven't checked to see if it does.
| # Windows configuration files should go to the Appdata\Roaming dir. | ||
| config_dir = appdirs.user_config_dir(APP_NAME, appauthor=False, roaming=True) |
There was a problem hiding this comment.
I wonder if it should be checking roaming first, but then check local as a fallback?
I think this change would break my autopkg prefs on my windows devices until I moved the prefs file from local to roaming to account for this change.
|
Honestly, I would prefer not to use an equivalent to xattr on Windows. Let's keep it simple and just store the etag information in a JSON file on disk, and read/write to it. Embedding it in the download just to mimic what it does on macOS using extended attributes seems unnecessarily difficult. |
|
This currently requires switching over to URLDownloaderPython on windows and non windows in order to achieve the JSON option, but an equivalent JSON file could be used as a fallback on URLDownloader as well, not just on windows, but on any file system that doesn't support xattr. I kind of prefer checking if xattr is supported, and if it is, using it, but if not, then use JSON, instead of checking |
I see your point. |
|
I'd like to potentially revisit this concept for a future AutoPkg release. Given what we have in 3.0.0RC1 right now, what would need to be done still to accommodate this? |
|
did not follow close on your work in the last few weeks. |
|
Looks like, the PR would work as expected. |
Implementing xattr support on Windows by gluing ETAG to File with ADS (alternate data stream).
Changing the config file loacation to %APPDATA%, instead of %LOCALAPPDATA%. This change is not strictly necessary, but would be good policy.