search

packit / dist-git-to-source-git

Converting dist-git to source-git
MIT License
4 stars 9 forks source link

get_local_specfile_path() now accepts Path instead of List[str] #3

Closed jpopelka closed 4 years ago

jpopelka commented 4 years ago

Needs https://github.com/packit-service/packit/pull/769

csomh commented 4 years ago

What is the order of discovery in get_local_specfile_path()? In case there are multiple *.spec files in the tree, is it guaranteed that it's always the same one found?

The reason for asking is b/c rpm for example has more than one spec file stored in their tree (in tests/data/SPECS) and probably they are not the ones we would like to find. get_local_specfile_path(DOWNSTREAM_FILES_DIR) would be probably safer.

jpopelka commented 4 years ago

The reason for asking is b/c rpm for example has more than one spec file stored in their tree (in tests/data/SPECS) and probably they are not the ones we would like to find.

see https://github.com/packit-service/packit/pull/769/commits/fb63b60517f7b415dbb486e2ae9467dd708f08c2

csomh commented 4 years ago

see packit-service/packit@fb63b60

Couldn't this be done without a change in get_local_specfile_path? For one, the change above is not backward compatible, and it only covers the example above. What if there are other cases?

The point is: it might be difficult to make assumptions about the content of the repository, besides the fact, that the SPEC-file we would like to use is in DOWNSTREAM_FILES_DIR. Explicitly excluding all the directories which might contain SPEC-files we don't care about might be more difficult then explicitly including the one we expect to contain the SPEC-file we want.

jpopelka commented 4 years ago

get_local_specfile_path(DOWNSTREAM_FILES_DIR) would be probably safer.

Where? Instead of get_local_specfile_path(specdir)? (might be better to comment the code next time so that I at least know which commit are we discussing here)

Couldn't this be done without a change in get_local_specfile_path?

We need that change anyway. But we can have our own get_local_specfile_path() here and put whatever you want in it. (I don't like depending on packit because of one tiny function anyway)

For one, the change above is not backward compatible

hence this PR. Decoupling from packit (as suggested above) would solve that problem.

, and it only covers the example above. What if there are other cases?

We won't discover them if we don't try.

The point is: it might be difficult to make assumptions about the content of the repository, besides the fact, that the SPEC-file we would like to use is in DOWNSTREAM_FILES_DIR. Explicitly excluding all the directories which might contain SPEC-files we don't care about might be more difficult then explicitly including the one we expect to contain the SPEC-file we want.

I'm sorry, are we discussing first or second commit?

If first, then: our aim is to remove the need for .packit.yaml completely so we need to have some auto-discovery mechanism and "explicitly including the one ..." is not possible when it's supposed to be "auto".

csomh commented 4 years ago

get_local_specfile_path(DOWNSTREAM_FILES_DIR) would be probably safer.

Where? Instead of get_local_specfile_path(specdir)? (might be better to comment the code next time so that I at least know which commit are we discussing here)

sorry, I totally missed line 253, and thought that this PR tries to entirely remove the usage of get_local_specfile_path().

Which makes most of my comments above pointless.

Couldn't this be done without a change in get_local_specfile_path?

We need that change anyway. But we can have our own get_local_specfile_path() here and put whatever you want in it. (I don't like depending on packit because of one tiny function anyway)

For one, the change above is not backward compatible

hence this PR. Decoupling from packit (as suggested above) would solve that problem.

The backward incompatible change in the packit PR you linked is the introduction of the exclude argument with a non-empty default. I don't think that change is needed. (This is not related to this PR).

If first, then: our aim is to remove the need for .packit.yaml completely so we need to have some auto-discovery mechanism and "explicitly including the one ..." is not possible when it's supposed to be "auto".

Yes, I know about that. Couldn't that be solved by an implicit .packit.yaml, configurable on a per gitforge basis in Packit Service? (But this comment has nothing to do with this PR).

jpopelka commented 4 years ago

I don't think that change is needed.

This discussion belongs into https://github.com/packit-service/packit/pull/769 but, look, it works with source-git-rpm (with no specfile_path in .packit.yaml) so until we find a case where it doesn't work I don't see a reason why not to have it.

Couldn't that be solved by an implicit .packit.yaml, configurable on a per gitforge basis in Packit Service?

Yes, that might be one of the solutions. Looks like a architecture meeting topic when the time comes.

jpopelka commented 4 years ago

I've split the PR. This one can be merged after https://github.com/packit-service/packit/pull/769 and #4 can be merged once https://github.com/packit-service/packit/pull/769 is in prod.