Skip to content
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

Versioned source tarball not available through github #111

Closed
matthijskooijman opened this issue May 1, 2020 · 7 comments
Closed

Versioned source tarball not available through github #111

matthijskooijman opened this issue May 1, 2020 · 7 comments
Assignees

Comments

@matthijskooijman
Copy link
Contributor

@matthijskooijman matthijskooijman commented May 1, 2020

I just tried to update the Debian package, using the github-generated tarball, but I ran into problems there because there is no nml/__version__py inside (since the tarball is just a direct snapshot from the git repo).

It seems pypi does have the correct tarball (as generated by the release flow), so I could check for updates there, but I suspect people might be looking at the releases tab in github there. So maybe the same tarball should be published on Github?

If that sounds good, I guess the release flow should be updated.

For the Debian package, it would also be good if the existing tarball from pypi could be added to the 0.5.0 release, so I can update my watch file accordingly (the filename should remain consistent with what the release flow will use, but the nml-0.5.0.tar.gz filename sounds fine? Or would you want a more explicit nml-source-0.5.0.tar.gz maybe?).

If this could be settled in the coming two days, that would be awesome, then I can still include in the upload for 0.5.0.

@matthijskooijman
Copy link
Contributor Author

@matthijskooijman matthijskooijman commented May 1, 2020

Hm, hold on - it seems the tarball on pypi is actually quite different from the git source. I suspect there might be some line endings translation? THat might actually be problematic, let me investigate.

@matthijskooijman
Copy link
Contributor Author

@matthijskooijman matthijskooijman commented May 1, 2020

Ok, the problematic changes are indeed only the line endings, which are Unix LF in 0.4.5 and git, and Windows CRLF in 0.5.0 on pypi. If I revert those line endings, the pypi tarball looks the same (in terms of which files are and are not present) to the 0.4.5 tarball, so that's good.

As for these line endings: I suspect this is because the source tarball is generated in the workflow on a Windows system, and the default git settings apparently translate line endings to the native format.

To fix this either:

  • just generate the source tarball on Linux
  • tell git to not touch line endings (not supported by the checkout workflow action directly (yet?), but there is a workaround just calling git config.
  • use .gitattributes to specify the default for the entire repo (which will also effect files in regular checkouts, so this might not be ideal, though it will have the most consistent result across all checkouts).

Regardless, I think this might need a new release, since the current 0.5.0 tarball is not entirely usable for Debian (I could use it, but then I'd get a ton of changes in my repo which would be reverted on the next release). If possible, I'd rather switch 0.5.0 and upload a to be released 0.5.1 (or 0.5.0.1 or something) with the fixed line endings...

@LordAro
Copy link
Member

@LordAro LordAro commented May 1, 2020

Hrm.

Generating the source tarball on Linux is tricky, as it uses a completely different method of building for the purposes of generating the wheels - hence why it's done on the Windows workflow currently. I guess I could quite easily switch it to the Mac workflow, which would be LF just the same as Linux.

I don't really want to set gitattributes to anything other than text=auto anyway, as IMO that's the Correct Thing To Do

@matthijskooijman
Copy link
Contributor Author

@matthijskooijman matthijskooijman commented May 1, 2020

Generating the source tarball on Linux is tricky, as it uses a completely different method of building for the purposes of generating the wheels

Really? I see this manylinux call, but there is also a plain setup.py sdist there:

python setup.py sdist bdist_wheel

But maybe the wheel-stuff already changed things in the checkout that messes with the source tree? Even then, the sdist could be built first? But using Mac is also fine, I expect.

I don't really want to set gitattributes to anything other than text=auto anyway, as IMO that's the Correct Thing To Do

I think you can also set some attribute to not do translation, but I'm not entirely sure. Just switching where the build happens is probably easier indeed.

LordAro added a commit to LordAro/nml that referenced this issue May 1, 2020
@matthijskooijman
Copy link
Contributor Author

@matthijskooijman matthijskooijman commented May 1, 2020

It seems that version.py-writing also gets CRLF on Windows, due to the default value of the newline argument to open(): https://docs.python.org/release/3.2/library/functions.html#open

This would also be solved for the source tarball if it were generated on something else than windows, but in general it might be useful to disable newline translation for __version__.py by passing newline='' to open?

@glx22
Copy link
Contributor

@glx22 glx22 commented May 1, 2020

I guess #108 could also upload source package to github release

@matthijskooijman
Copy link
Contributor Author

@matthijskooijman matthijskooijman commented May 1, 2020

I noticed one more thing with the tarball: files like setup.py and nmlc are no longer executable. I suspect this will also be fixed when the tarball is generated on OSX instead.

Would there be a way to preview the tarball generated from #113? Then I can maybe see if the Debian package builds with that (though that also needs #112 fixed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants