Add output function

[Korusuke]

It's not yet totally complete as ruby PR is yet to be merged.

[Korusuke]

When trying to output to a file as of now it is throwing an error if the file already exists and the program stops. I added recursive functionality to my local copy and when experimenting with that this was the biggest issue, like if we try packaging upt-cpan then it will add requests too and requests is also a dependency for upt-pypi so when trying to recursively package that it throws an error and stop there.

This could be a potential issue. We can either just log the error and continue executing or we can create .new file in case the file already exists.

Also, another thing not related to this PR that I wanted to confirm, while recursive packaging do we need to check the MacPorts tree that whether the ports already exist or not ?

[reneeotten]

When trying to output to a file as of now it is throwing an error if the file already exists and the program stops.

Yes, and that's fine for this PR where the point is to write the output of one package to a file. Indeed it will need some more considerations later in the context of both recursive addition and updating. That discussion does not really belong in this PR though, so please open a new issue for this or start a discussion in the PR where it applies to.

I added recursive functionality to my local copy and when experimenting with that this was the biggest issue, like if we try packaging upt-cpan then it will add requests too and requests is also a dependency for upt-pypi so when trying to recursively package that it throws an error and stop there.

This could be a potential issue. We can either just log the error and continue executing or we can create .new file in case the file already exists.

Also, another thing not related to this PR that I wanted to confirm, while recursive packaging do we need to check the MacPorts tree that whether the ports already exist or not ?

Short answer: yes, I would prefer to check if a package is already present (and whether the available version can fulfill the requirement). If so, I think it can be ignored from the perspective of "recursive packaging" - if you want to be pro-active one could check if it's actually the most recent version that is present and log a message if that's not the case.

[Steap]

When trying to output to a file as of now it is throwing an error if the file already exists and the program stops. I added recursive functionality to my local copy and when experimenting with that this was the biggest issue, like if we try packaging upt-cpan then it will add requests too and requests is also a dependency for upt-pypi so when trying to recursively package that it throws an error and stop there.

I'm pretty sure we will want the backend to tell upt whether the package already exists.

[reneeotten]

I'm pretty sure we will want the backend to tell upt whether the package already exists.

I can imagine that a developer wants to just generate a new Portfile (e.g., for comparison) and in that case upt should just print it to the screen even if the port already exists. So when using the "output" feature I suppose that the backend could check whether the port exists before attempting to package it again and then fail at the output stage, but for one port it actually doesn't take any time - so I would be fine with just checking it at then end when trying to write the file.

I would prefer to change what happens during the except FileExistsError: to not do a sys.exit() but just log that the directory already exists and revert to printing the Portfile to stdout.

Nevertheless, in the long run for "recursive packaging" and the "update feature", we'll probably need to handle the situation where a port already exists differently and, for example, do an "update" in that case even if the end-user did "package" to start with.

@Korusuke @Steap what do you think, would this be a good (temporary) solution?

[reneeotten]

ping @Steap @Korusuke see my comment above, what do you think about that suggestion?

[Steap]

I think most of this is up to the various communities, and how they usually work.

Nevertheless, in the long run for "recursive packaging" and the "update feature", we'll probably need to handle the situation where a port already exists differently and, for example, do an "update" in that case even if the end-user did "package" to start with.

I'm not a big fan of this. The update feature is probably going to be kind of experimental for a while, so I would rather not update anything when the "--update" is not given.

[reneeotten]

I think most of this is up to the various communities, and how they usually work.

Nevertheless, in the long run for "recursive packaging" and the "update feature", we'll probably need to handle the situation where a port already exists differently and, for example, do an "update" in that case even if the end-user did "package" to start with.

I'm not a big fan of this. The update feature is probably going to be kind of experimental for a while, so I would rather not update anything when the "--update" is not given.

@Steap, sorry for not being clear - I completely agree with your statement. What I meant to say is that once we have added the "--update" options we can of course not stop (or only print the Portfile to stdout) anymore when a port already exists.

In other words, this PR looks good to me after the change below and we can merge this; we will need to revisit this when implementing the "update" feature.

I would prefer to change what happens during the except FileExistsError: to not do a sys.exit() but just log that the directory already exists and revert to printing the Portfile to stdout.

[mojca]

Just a random thought (not too relevant for this PR).

I'm currently spending waaaaay too much time just trying to get one single port packaged (and trying to be a good citizen by adding test.run yes to the ports when I can).

Complaining that upt gets dependencies wrong in most of the cases is a subject for another evening, I assume we could leverage this project "create a CI system for pypi packages" :), but what would definitely help would be upt telling me that "hey, these dependencies are outdated" (including recursive test dependencies). Sure, there could be an option to update ports at some point in the future, but already the info that something needs to be updated would be helpful. It would also help if we could get a warning when a particular version of dependency is not compatible (usually too old) or a dependent port requires an older version. (Getting a warning when you update a port to a version that's no longer compatible with a dependent port could be nice, but probably require a disproportionate amount of effort.)

Sadly port livecheck rdepof:py37-foo doesn't work because livechecks are disabled for subports and main port doesn't have the dependencies declared. But maybe there's a way to fix that (something to be discussed by the community).

[reneeotten]

In other words, this PR looks good to me after the change below and we can merge this; we will need to revisit this when implementing the "update" feature.

I would prefer to change what happens during the except FileExistsError: to not do a sys.exit() but just log that the directory already exists and revert to printing the Portfile to stdout.

I am clearly not really thinking before typing.... I think it's fine like it is. One can, of course, just skip the "--output" and it will just print things to stdout ;)

[reneeotten]

Just a random thought (not too relevant for this PR).

I'm currently spending waaaaay too much time just trying to get one single port packaged (and trying to be a good citizen by adding test.run yes to the ports when I can).

Yes, it does take time to package something properly and be a good citizen.... but, without upt it would take probably even more time ;)

Complaining that upt gets dependencies wrong in most of the cases is a subject for another evening, I assume we could leverage this project "create a CI system for pypi packages" :),

Well, unfortunately that's mainly upstream and/or PyPI their fault and not upt - unless you're saying that pypi2port produces better results; in that case we should certainly try and improve the current status of upt-macports.

but what would definitely help would be upt telling me that "hey, these dependencies are outdated" (including recursive test dependencies). Sure, there could be an option to update ports at some point in the future, but already the info that something needs to be updated would be helpful. It would also help if we could get a warning when a particular version of dependency is not compatible (usually too old) or a dependent port requires an older version. (Getting a warning when you update a port to a version that's no longer compatible with a dependent port could be nice, but probably require a disproportionate amount of effort.)

Much of what you're discussing here is part of the "recursive packaging" feature that is being discussed/implemented in #32 and #35. I appears that in general the specifiers for versioning for all frontends isn't that robust (yet), but I think for PyPI we should be able to most of what you're suggesting.

[Korusuke]

I am clearly not really thinking before typing.... I think it's fine like it is. One can, of course, just skip the "--output" and it will just print things to stdout ;)

So I guess this PR is ready to be merged or should we change the sys.exit to just logging as this will be needed in recursive packaging?

[reneeotten]

okay, I am going to merge this by tomorrow morning (so in let's say 15 hours from now) unless I hear otherwise! Confirming that you (@Steap) agree with it (or just merging it yourself) would shorten this timeline ;)

[Steap]

My main issue is that there are no tests here. Creating directories, making sure we do not overwrite anything, etc.: these actions deserve some testing.

[reneeotten]

My main issue is that there are no tests here. Creating directories, making sure we do not overwrite anything, etc.: these actions deserve some testing.

I didn't look at that, but it's a good point - the coverage would indeed go down from 84 to 71%. So the output function itself looks good when we're adding a single port and to get this finished @Korusuke please add tests. Also, please rebase with master and resolve possible conflicts.

After that we can move on to the recursive part of the packaging, and then the output function might need some additional tweaks but that should not stop us from finishing up this PR now.

[reneeotten]

I do not have much experience with mock, but reading a bit through the documentation and your tests it seems that it could work like this. You're now "mimicking/mocking" situations pretending that you would get a PermissionError or FileExistsError. I would probably have made a temporary directory and made sure that you generate the correct directory/Portfile and that the proper errors are raised where you want them to (right now you only test whether a SystemExit happens), but in the end it might be equivalent.

One situation that isn't handled now is the following:

• directory already exists (for some reason already made by the end-user), but there is no file in there.

Currently, it will throw a FileExistsError which actually just checks if the directory exists; this check could perhaps be expanded?

@Steap do you have (more) experience with mock and are these tests sufficient?

[Steap]

@reneeotten os.makedirs takes an optional argument: exist_ok. If set to True, os.makedirs will not fail if a directory already exists. By default, it is set to False.

Other than that, the tests look ok.

[Korusuke]

@reneeotten os.makedirs takes an optional argument: exist_ok. If set to True, os.makedirs will not fail if a directory already exists. By default, it is set to False.

Other than that, the tests look ok.

Just to make sure, here it means that if the folder structure already exists then don't throw an error but then if file exists then do throw an error, right?

[reneeotten]

@reneeotten os.makedirs takes an optional argument: exist_ok. If set to True, os.makedirs will not fail if a directory already exists. By default, it is set to False. Other than that, the tests look ok.

Just to make sure, here it means that if the folder structure already exists then don't throw an error but then if file exists then do throw an error, right?

@Steap @Korusuke yes I am aware of the exist_ok argument for os.makedirs and that would probably be a good idea to use. One could catch then only the PermissionError and do an additional check if a Portfile exists in the ouput_dir.

The most important check is whether the Portfile is present and if so to make sure we don't overwrite anything. I would be in favor of continuing if for some reason the end-user already made the directory, but there isn't a Portfile yet.

[reneeotten]

okay, in it goes!