MSI: Reorganize payload preparation#1164
MSI: Reorganize payload preparation#1164lrandersson wants to merge 3 commits intoconda:briefcase-integrationfrom
Conversation
conda-bot
added
the
cla-signed
|
I'm tagging @marcoesters here because I assume you will review this out of the possible reviewers. I just wanted to inform you that there is an additional PR that depends on this one, that also is in need of review. lrandersson#2, this PR is on my own fork because I need to merge that PR into this one (PR 2 -> PR 1164). |
| def create(info, verbose=False): | ||
| if not IS_WINDOWS: | ||
| raise Exception(f"Invalid platform '{sys.platform}'. MSI installers require Windows.") | ||
| @dataclass(frozen=True) |
There was a problem hiding this comment.
Why use a dataclass over NamedTuple?
There was a problem hiding this comment.
I'd say that they are very similar but in my opinion this more comes down to how we use this. The class PayloadLayout represents one thing, and we can add methods to it as well. I tried to keep it minimal for presentation. For a tuple also positional meaning is primary, but for the frozen data class the fields are primary.
There was a problem hiding this comment.
I'm suggesting a NamedTuple, not a tuple. A NamedTuple also goes by fields and is immutable. I don't see that we will need methods for a structure that just describes a layout.
There was a problem hiding this comment.
I see, what would the gain be to use a NamedTuple over a dataclass in this situation? I agree that we still get the data organized in a similar fashion but they signal different intent, and my thought process is:
- I'd use a
NamedTuplewhen the thing is a tuple. - I'd use frozen dataclass when the thing represents a thing.
There was a problem hiding this comment.
Overall, a NamedTuple is a smaller object. I use a NamedTuple when I just need a simple immutable data container. However, I'm still not fully convinced we need this (yet). See my comment on the perpare function below.
| def remove(self) -> None: | ||
| shutil.rmtree(self.root) |
There was a problem hiding this comment.
Should there be a guard in case the directory doesn't exist (or an ignore_errors=True)?
There was a problem hiding this comment.
I thought about it, but, I think it would make the most sense that the one who implements the interface is the one who should care about potential errors. Silently catching/ignoring errors here could lead to unexpected behavior.
There was a problem hiding this comment.
By the way I have now guards for this since Payload.root is now a cached property in lrandersson#2
| def _ensure_root(self) -> Path: | ||
| if self.root is None: | ||
| self.root = Path(tempfile.mkdtemp()) | ||
| return self.root |
There was a problem hiding this comment.
This should be part of __init__ or we shouldn't allow None root. I don't see an advantage having a potentially "unbound" root directory.
There was a problem hiding this comment.
That is an option, the reason why I have done as above is to avoid filesystem side effects upon class instantiation. The caller ideally only need to use this as:
payload = Payload(info)
payload.prepare()
However, if we are fine with above the side effects, let me know and I will update it. It is a minor thing to change in terms of code.
| self._stage_dists(layout) | ||
| self._stage_conda(layout) | ||
| return layout |
There was a problem hiding this comment.
This part makes me think that PayloadLayout may be overkill. _stage_dists and _stage_conda only use one element of PayloadLayout and only root is used outside of that function, so the entire object doesn't need to be returned.
Why not combine _create_layout and prepare into one function and remove the PayloadLayout data class? This seems simpler for now.
If we find that we want to use this class later for other installers, we can create these interfaces later.
There was a problem hiding this comment.
I see your point, but I think it'll be a bit more "clear" after reviewing the follow-up PR https://github.com/lrandersson/constructor/pull/2/changes where more code has been added.
I like to think about it this way, alternative 1 (which we are very close to in the PR I linked):
self.render_templates(layout)
self.write_pyproject_toml(layout)
self.archive(layout)
self._stage_dists(layout)
self._stage_conda(layout)
Compared to alternative 2:
external = root / "external"
base = external / "base"
pkgs = base / "pkgs"
self.render_templates(root, external)
self.write_pyproject_toml(root, external)
self.archive(base, external)
self._stage_dists(pkgs)
self._stage_conda(external)
And if for some reason we need to add more directories as arguments to any of the functions, one does not need to change the function definitions, only the implementation of each function.
To me it doesn't seem overkill but for maintenance and ease of implementation, alternative 1 is much simpler. Please let me know your thoughts.
There was a problem hiding this comment.
Alternative 1 already lists a few MSI-specific function calls, like preparing the pyproject.toml file (which is technically not part of the installer payload anyway). The SH format structures its payload differently (preconda, postconda), and EXE installers have temp_extra_files.
I am overall in favor of a more object-oriented way rather than the current scripting appraoch constructor have, and alternative 1 looks reasonable at first glance.
However, I am concerned about introducing premature abstraction now that we may have to undo later. I think for now, alternative 2 is good enough for MSI. When there is earnest effort in making constructor more OOP-based, we can take a holistic look at all payloads and find what an appropriate level of abstraction is.
There was a problem hiding this comment.
I see your point. Do you mind if we take this action as the last thing after merging lrandersson#2 into this PR? That way we can also see a simple diff between the two layouts given the current implementation.
(It will also be easier for me to merge the second PR that way).
| """The layout is created as: | ||
| root/ | ||
| └── external/ | ||
| └── base/ | ||
| └── pkgs/ | ||
| """ |
There was a problem hiding this comment.
I think this is a good opportunity to elaborate a little more on why this directory structure, specifically the intermediary external directory is needed.
There was a problem hiding this comment.
Added here c4b11e8, note that there is also a path to external in the pyproject.toml that is generated, but I didn't explicitly mention that in the docstring.
3 participants
Description
This PR proposes a class
Payloadto organize the payload preparation.I still think more work can be done on this but I tried to keep the changes minimal to simplify the review.
I have also added a couple of unit tests to demonstrate what I think is the biggest gain of using this structure over the old one.
I think this class can be extracted later and also be used for other installers, since we can make it possible (if needed) to override the function that creates the payload layout.
For the review, it might be easier to also open the whole file via "View file" and not only browsing from the "Files changed" tab.
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?