Skip to content

ft: Instantiate Specfile from strings or file-like objects #458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 5, 2025

Conversation

mynk8
Copy link
Contributor

@mynk8 mynk8 commented Mar 12, 2025

TODO:

  • Write new tests or update the old ones to cover new functionality.
  • Update doc-strings where appropriate.
  • Update or write new documentation.

Fixes #206
Fixes #248

RELEASE NOTES BEGIN

  • Added support for creating Specfile instances from file objects and strings.

RELEASE NOTES END

Copy link
Contributor

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/d3c72d139e844076807086e50e1a73b2

✔️ pre-commit SUCCESS in 2m 00s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 03s
✔️ specfile-tests-pip-deps SUCCESS in 58s

@mynk8 mynk8 changed the title ft: Instantiate specfile from string ft: Add method to Specfile to Instantiate from string Mar 13, 2025
@nforro
Copy link
Member

nforro commented Mar 14, 2025

Thank you for the contribution!
However, I think this problem could be handled in a more elegant way, without using temporary files, and together with #206. The idea is to replace the internal Specfile._path attribute with a file-like object that could be either passed directly to the constructor or instantiated from a path using open() or from a string using StringIO.
The implementation would simply replace instances of Specfile._path.read_text() with something like Specfile._stream.read() and instances of Specfile._path.write_text() with something like Specfile._stream.write() if the file-like object is writable. It would also need to handle seeking, truncating, flushing etc. accordingly.

@mynk8
Copy link
Contributor Author

mynk8 commented Mar 14, 2025

I tried implementing as described and converted even the path to a fileobject (self._stream = self._path.open("r+"))
But two tests were failing:

FAILED tests/integration/test_specfile.py::test_copy - TypeError: cannot pickle 'TextIOWrapper' instances
FAILED tests/integration/test_specfile.py::test_parse_if_necessary - TypeError: cannot pickle 'TextIOWrapper' instances

and this will fail for objects instantiated from string and open() as well. Not really sure how to handle this.

@nforro
Copy link
Member

nforro commented Mar 14, 2025

Right, that will require doing something similar to what is done in SpecParser:

def __deepcopy__(self, memo: Dict[int, Any]) -> "SpecParser":
result = self.__class__.__new__(self.__class__)
memo[self.id()] = result
for k, v in self.__dict__.items():
if k in ["spec", "tainted"]:
continue
setattr(result, k, copy.deepcopy(v, memo))
result.spec = None
result.tainted = False
return result

The _stream attribute will have to be skipped when deepcopying a Specfile instance, but the question is what should the copy get. For Specfile instantiated from a path it could be calling open() again on the same path, for Specfile instantiated from a string it could be creating a new StringIO instance and supplying it with the content of the original StringIO. For Specfile instantiated with a file-like object perhaps we could just disable deepcopying.

Copy link
Contributor

@mynk8
Copy link
Contributor Author

mynk8 commented Mar 14, 2025

The tests are passing now. Currently:

  • Return a new StringIO instance for the ones instantiated from it.
  • Return a copy for the ones instantiated from path. They are still file-like objects from open().
  • Ignore and return None for file-like objects without path.

will write new tests for them.

Copy link
Contributor

Copy link
Member

@nforro nforro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this looks much better.

Copy link
Contributor

Copy link
Contributor

@mynk8
Copy link
Contributor Author

mynk8 commented Mar 15, 2025

I had a doubt in the reload function:

    def reload(self) -> None:
        """Reloads the spec file content."""
        self._lines, self._trailing_newline = self._read_lines(self._file)

Why we are not parsing again here ? I think this causes the specfile object to be inconsistent after a reload.

Copy link
Contributor

@nforro
Copy link
Member

nforro commented Mar 15, 2025

Why we are not parsing again here ? I think this causes the specfile object to be inconsistent after a reload.

By parsing do you mean calling self._parser.parse()? Because it would be unnecessary, it is called before any data that depend on the result of RPM parsing are accessed.

@mynk8
Copy link
Contributor Author

mynk8 commented Mar 15, 2025

Why we are not parsing again here ? I think this causes the specfile object to be inconsistent after a reload.

By parsing do you mean calling self._parser.parse()? Because it would be unnecessary, it is called before any data that depend on the result of RPM parsing are accessed.

yes, I was talking about that. It's clear now

@mynk8 mynk8 changed the title ft: Add method to Specfile to Instantiate from string ft: Instantiate Specfile from strings or file-like objects Mar 15, 2025
Copy link
Contributor

Copy link
Member

@nforro nforro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's starting to look really good, thanks!
Could you please squash the commits into one (or a couple if you feel it's necessary)?
It would also be nice to check why the source-git tests in packit integration test suite are failing and if it is related to these changes.

Copy link
Contributor

@mynk8
Copy link
Contributor Author

mynk8 commented Mar 17, 2025

FAILED integration/test_source_git.py::test_basic_local_update_without_patching
FAILED integration/test_source_git.py::test_basic_local_update_empty_patch[None]
FAILED integration/test_source_git.py::test_basic_local_update_empty_patch[0.1.0]
FAILED integration/test_source_git.py::test_basic_local_update_empty_patch[0.1*]
FAILED integration/test_source_git.py::test_basic_local_update_empty_patch[0.*]
FAILED integration/test_source_git.py::test_basic_local_update_patch_content
FAILED integration/test_source_git.py::test_basic_local_update_patch_content_with_metadata
FAILED integration/test_source_git.py::test_basic_local_update_patch_content_with_metadata_and_patch_ignored
FAILED integration/test_source_git.py::test_basic_local_update_patch_content_with_downstream_patch
FAILED integration/test_source_git_status.py::test_source_git_status_synced[check_ready_api_dg_first]
FAILED integration/test_source_git_status.py::test_source_git_status_synced[check_ready_api_sg_first]
FAILED integration/test_source_git_status.py::test_source_git_status_dist_git_ahead[check_ready_api_dg_first]
FAILED integration/test_source_git_status.py::test_source_git_status_dist_git_ahead[check_ready_api_sg_first]
FAILED integration/test_source_git_status.py::test_source_git_status_source_git_ahead[check_ready_api_dg_first]
FAILED integration/test_source_git_status.py::test_source_git_status_source_git_ahead[check_ready_api_sg_first]
FAILED integration/test_source_git_status.py::test_source_git_status_history_diverges[check_ready_api_dg_first]
FAILED integration/test_source_git_status.py::test_source_git_status_history_diverges[check_ready_api_sg_first]
FAILED integration/test_upstream.py::test_fix_spec_persists - AssertionError:...

These are the failing tests, I think the last one might have something to do with this PR. I am having trouble squashing commits from my side they seem to be getting duplicated.

@nforro
Copy link
Member

nforro commented Mar 17, 2025

These are the failing tests, I think the last one might have something to do with this PR

No, all of the failures are related to this PR. I haven't check all of them but they all most likely fail for the same reason - the spec file is replaced with a different file, so the _file object represents a deleted file and reload() doesn't read contents of the new file. I think that's fine, but it will be necessary to add a new method, called for example reopen(), that will reopen the path, but do nothing on other types of file-like objects.

That brings me to another step that will be necessary and that's adding tests covering every new feature and changed behavior, including this one.

@nforro
Copy link
Member

nforro commented Mar 17, 2025

I am having trouble squashing commits from my side they seem to be getting duplicated.

https://medium.com/@slamflipstrom/a-beginners-guide-to-squashing-commits-with-git-rebase-8185cf6e62ec

@nforro
Copy link
Member

nforro commented Mar 17, 2025

I think that's fine, but it will be necessary to add a new method, called for example reopen(), that will reopen the path, but do nothing on other types of file-like objects.

Actually, thinking about it, it should rather be done in reload(), in order not to change the behavior for existing users.

@mynk8
Copy link
Contributor Author

mynk8 commented Mar 17, 2025

I am having trouble squashing commits from my side they seem to be getting duplicated.

https://medium.com/@slamflipstrom/a-beginners-guide-to-squashing-commits-with-git-rebase-8185cf6e62ec

Got it done eventually i was avoiding force push and rewriting the commits.
Yes, thinking about it, the issue seems to be with reload. There was an assertion error in a test, where it still had the old version string.

Copy link
Contributor

@mynk8
Copy link
Contributor Author

mynk8 commented Mar 17, 2025

These are the failing tests, I think the last one might have something to do with this PR

No, all of the failures are related to this PR. I haven't check all of them but they all most likely fail for the same reason - the spec file is replaced with a different file, so the _file object represents a deleted file and reload() doesn't read contents of the new file. I think that's fine, but it will be necessary to add a new method, called for example reopen(), that will reopen the path, but do nothing on other types of file-like objects.

That brings me to another step that will be necessary and that's adding tests covering every new feature and changed behavior, including this one.

I think the current test cases should be reused, but with different input types, such as a raw string, a file-based input, and a file without a 'name' attribute, it might be overkill though

Comment on lines 15 to 23


try:
from typing import TypedDict
except ImportError:
from typing_extensions import TypedDict

EncodingArgs = TypedDict("EncodingArgs", {"encoding": str, "errors": str})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, black doesn't seem to care, but I think it would make more sense to either move the double newline:

Suggested change
try:
from typing import TypedDict
except ImportError:
from typing_extensions import TypedDict
EncodingArgs = TypedDict("EncodingArgs", {"encoding": str, "errors": str})
try:
from typing import TypedDict
except ImportError:
from typing_extensions import TypedDict
EncodingArgs = TypedDict("EncodingArgs", {"encoding": str, "errors": str})

or remove it:

Suggested change
try:
from typing import TypedDict
except ImportError:
from typing_extensions import TypedDict
EncodingArgs = TypedDict("EncodingArgs", {"encoding": str, "errors": str})
try:
from typing import TypedDict
except ImportError:
from typing_extensions import TypedDict
EncodingArgs = TypedDict("EncodingArgs", {"encoding": str, "errors": str})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shows as resolved, but nothing has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i forgot to commit that change my bad

Comment on lines +167 to +173
# IO doesn't implement getvalue() so tell mypy this is StringIO
# (could also be BytesIO)
sio = cast(StringIO, self._file)
# not a named file, try `getvalue()`
specfile._file = type(sio)(sio.getvalue())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this, but I don't know how to do this better without actually checking for type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also a type ignore, but I think that's worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any alternative method so far.

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/bb97922bdd16403f815e2972dbc6e0a9

✔️ pre-commit SUCCESS in 2m 00s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 52s
✔️ specfile-tests-pip-deps SUCCESS in 1m 48s

@nforro nforro self-assigned this Apr 3, 2025
Copy link
Member

@nforro nforro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work on this!
Could you please squash the commits and update the release notes?

Comment on lines 84 to 87
path: Path to the spec file.
file: File object representing the spec file.
content: String containing the spec file content.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we document the fact that these arguments are mutually exclusive? Probably in the description above, to avoid repetition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, maybe it would make sense to change the order and swap file and content, file is on a lower level than path and content and we can't move path for backward compatibility.

Comment on lines 15 to 23


try:
from typing import TypedDict
except ImportError:
from typing_extensions import TypedDict

EncodingArgs = TypedDict("EncodingArgs", {"encoding": str, "errors": str})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shows as resolved, but nothing has changed.

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/f17c6af5d06f407895767ef3e994e300

✔️ pre-commit SUCCESS in 1m 54s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 49s
✔️ specfile-tests-pip-deps SUCCESS in 1m 45s

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/d29135ab1a464d33b62359d0bc711337

✔️ pre-commit SUCCESS in 1m 59s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 47s
✔️ specfile-tests-pip-deps SUCCESS in 1m 46s

softwarefactory-project-zuul bot added a commit to packit/packit that referenced this pull request Apr 4, 2025
… fixture (#2580)

Initiate dist-git repo after spec file is present in `cockpit_ostree` fixture

The spec file is copied to the dist-git repo after it is initialized, which means it isn't commited to git and eventually gets removed, but that doesn't make the tests fail because the related Specfile instance keeps its content in memory even though the file no longer exists. That however changes once the Specfile instance gets properly reloaded when needed and the tests start to fail. Fix that.
Related to #2532, packit/specfile#458.

Reviewed-by: Laura Barcziová
Reviewed-by: Matej Focko
@nforro
Copy link
Member

nforro commented Apr 4, 2025

/packit build

@nforro
Copy link
Member

nforro commented Apr 4, 2025

/packit-stg build

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/ed775370919c4135a401f6240d753be3

✔️ pre-commit SUCCESS in 2m 00s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 46s
✔️ specfile-tests-pip-deps SUCCESS in 1m 44s

@mynk8
Copy link
Contributor Author

mynk8 commented Apr 4, 2025

Thanks for actively guiding throughout the process !!
I learnt a lot working on this.

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/3ae9d9b327a24d1880ddfa29df50bfb8

✔️ pre-commit SUCCESS in 1m 54s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 46s
✔️ specfile-tests-pip-deps SUCCESS in 1m 42s

@mynk8 mynk8 force-pushed the string branch 2 times, most recently from 85de55e to 10904a6 Compare April 4, 2025 19:40
Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/14411b090483488daf5246f1d750163b

✔️ pre-commit SUCCESS in 1m 52s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 48s
✔️ specfile-tests-pip-deps SUCCESS in 1m 41s

sourcedir: Optional[Union[Path, str]] = None,
autosave: bool = False,
macros: Optional[List[Tuple[str, Optional[str]]]] = None,
force_parse: bool = False,
) -> None:
"""
Initializes a specfile object.

The arguments `path`, `file` and `content` are mutually exclusive and only one can be used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to apply the suggestion from the start of the docstring, nevertheless:

Suggested change
The arguments `path`, `file` and `content` are mutually exclusive and only one can be used.
Initializes a specfile object. You can specify either a path to the spec file,
its content as a string or a file object representing it. `sourcedir` is optional
if `path` or a named `file` is provided and will be set to the parent directory.

@nforro
Copy link
Member

nforro commented Apr 5, 2025

Could you modify the description a bit:

Fixes
Related to #206 and #248

It's not only related to those issues, it does fix them.

RELEASE NOTES BEGIN

  • Support creating Specfile instances from file-objects and strings.
  • reload() now reopens the file.
  • Update tests for running every test case with a fixture for different input modes, file_path, text_file, binary_file, text_io_stream, > binary_io_stream, content_string.
  • Add a test for reload().

RELEASE NOTES END

Release notes are supposed to describe user-facing changes. Added support for creating Specfile instances from file objects and strings. is just fine.

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/a23cd51632064845978478a06b921736

✔️ pre-commit SUCCESS in 2m 00s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 49s
✔️ specfile-tests-pip-deps SUCCESS in 1m 45s

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/c39f438384bd4af1bb81886b4b53953f

✔️ pre-commit SUCCESS in 2m 01s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 49s
✔️ specfile-tests-pip-deps SUCCESS in 1m 45s

@nforro
Copy link
Member

nforro commented Apr 5, 2025

Fixes #206 and #248

Actually, in order for GitHub to automatically close the fixed issues you have to list them separately, so one Fixes line per each. Note the Successfully merging this pull request may close these issues. section on the right.

@nforro
Copy link
Member

nforro commented Apr 5, 2025

/packit build

@nforro
Copy link
Member

nforro commented Apr 5, 2025

/packit-stg build

@nforro nforro added the mergeit Zuul, merge it! label Apr 5, 2025
Copy link
Contributor

Build succeeded (gate pipeline).
https://softwarefactory-project.io/zuul/t/packit-service/buildset/cd7b86b33ea14d818c6cb56723b1e068

✔️ pre-commit SUCCESS in 1m 58s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 6daed02 into packit:main Apr 5, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergeit Zuul, merge it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instantiate Specfile with a string Create Specfile from a virtual file
3 participants