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

Add Py 1.0 #11446

Merged
merged 6 commits into from Feb 20, 2018
Merged

Add Py 1.0 #11446

merged 6 commits into from Feb 20, 2018

Conversation

zshipko
Copy link
Contributor

@zshipko zshipko commented Feb 19, 2018

No description provided.

@camelus
Copy link
Contributor

camelus commented Feb 19, 2018

✅ All lint checks passed 1dfda4e
  • These packages passed lint tests: py.1.0

✅ Installability check (8323 → 8324)
  • new installable packages (1): py.1.0

@zshipko
Copy link
Contributor Author

zshipko commented Feb 19, 2018

It looks like m4 is missing on Alpine, or is this something I need to fix on my end?

@mseri
Copy link
Member

mseri commented Feb 19, 2018

I think it's more general, plenty of packages are failing in that way. This looks fine

"ctypes" {>= "0.13.0"}
"ctypes-foreign" {>= "0.4.0"}
]
depopts: []
Copy link
Member

@mseri mseri Feb 19, 2018

Choose a reason for hiding this comment

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

You can remove this line, it's necessary only if you have depopts (same for the tags)

@mseri
Copy link
Member

mseri commented Feb 19, 2018

I can see on the repository that you have tests but you don't have a build-test secion in the opam file to run them in the CI. Probably it's worth adding it

@zshipko
Copy link
Contributor Author

zshipko commented Feb 19, 2018

Okay, @mseri - I have made your suggested changes. Please let me know if there is anything else I should address before it's ready to merge.

Thanks.

@mseri
Copy link
Member

mseri commented Feb 19, 2018

Thanks! The tests now are failing. I think it's due to the missing python devel libraries. You should be able to fix it in most cases adding specific depexts to the opam file. I think the following should be enough

depexts: [
  [["debian"] ["python3-dev"]]
  [["ubuntu"] ["python3-dev"]]
  [["fedora"] ["python3-devel"]]
  [["centos"] ["python3-devel"]]
  [["alpine"] ["python3-dev"]]
]

@zshipko
Copy link
Contributor Author

zshipko commented Feb 19, 2018

@mseri, Thanks! I have updated the depexts, but it seems to be failing on CentOS now as well.

@mseri
Copy link
Member

mseri commented Feb 19, 2018

Mmh, it’s a bit trickier than that. The error now says

libpython3.4m.so: undefined symbol: Py_DecodeLocale

And from python’s documentation looks like this symbol has been introduced in python 3.5
I don’t think there is an easy way to restrict the depext to python > 3.5, I am not sure if you can/want to do anything about that. You could drop support for python3.4or maybe there is a way to define the call depending on the detected python version

@zshipko
Copy link
Contributor Author

zshipko commented Feb 19, 2018

Hmm, yeah I saw that. I don't think I want to support Python 3.4 right now. Thanks for your help.

@zshipko zshipko closed this Feb 20, 2018
@hcarty
Copy link
Member

hcarty commented Feb 20, 2018

@zshipko It's ok to only support Python 3.5. It may be better to leave out the depext for OS distributions which don't provide a recent enough Python version and include a note stating that Python 3.5 or later is required.

@zshipko zshipko reopened this Feb 20, 2018
@zshipko
Copy link
Contributor Author

zshipko commented Feb 20, 2018

@hcarty, Great! I've documented the requirement in the descr file. Is that sufficient?

@hcarty
Copy link
Member

hcarty commented Feb 20, 2018

You could add some extra messages for users (see https://github.com/ocaml/opam-repository/blob/master/packages/lwt/lwt.3.2.1/opam and https://github.com/ocaml/opam-repository/blob/master/packages/mysql/mysql.1.2.2/opam for examples). I think this is ok to merge as-is or with the extra messages. What is your preference @zshipko?

@zshipko
Copy link
Contributor Author

zshipko commented Feb 20, 2018

@hcarty I just added a message to the opam file.

@mseri
Copy link
Member

mseri commented Feb 20, 2018

Thanks! I think it’s fine to merge now. I’ll leave it open for comments a bit more and merge in the afternoon

@zshipko
Copy link
Contributor Author

zshipko commented Feb 20, 2018

@mseri Sounds good!

@hcarty
Copy link
Member

hcarty commented Feb 20, 2018

Agreed that this looks good. Thank you for your patience @zshipko!

@mseri mseri merged commit 1de3f5e into ocaml:master Feb 20, 2018
@mseri
Copy link
Member

mseri commented Feb 20, 2018

Thanks for the patience. Feel free to announce this release on https://discuss.ocaml.org, we have an Announce tag and a Community category (or maybe the other way around) for this purpose

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

Successfully merging this pull request may close these issues.

None yet

4 participants