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

Upgrade to 1.3+, use Expat_jll to vendor libexpat #99

Merged
merged 3 commits into from
Sep 12, 2019
Merged

Conversation

staticfloat
Copy link
Contributor

This will allow Julia 1.3 to stop shipping libexpat with the distribution; the library will be dynamically downloaded and unpacked when needed.

@codecov
Copy link

codecov bot commented Sep 10, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@256ca60). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #99   +/-   ##
=========================================
  Coverage          ?   30.21%           
=========================================
  Files             ?        4           
  Lines             ?     1026           
  Branches          ?        0           
=========================================
  Hits              ?      310           
  Misses            ?      716           
  Partials          ?        0
Impacted Files Coverage Δ
src/LibExpat.jl 81.36% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 256ca60...3b54291. Read the comment docs.

@staticfloat
Copy link
Contributor Author

Wow, that's quite the drop in coverage.

Manifest.toml Outdated
@@ -0,0 +1,63 @@
# This file is machine-generated - editing it directly is not advised

Copy link
Member

Choose a reason for hiding this comment

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

Better not ship the Manifest!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? Bundling the manifest is useful.

Copy link
Member

Choose a reason for hiding this comment

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

Is it? I've only had problems with it and thought it's a semi official recommendation ;)

Copy link
Member

Choose a reason for hiding this comment

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

@staticfloat I had problems with CI failing on HDF5.jl due to the checked in Manifest.toml file getting picked up and screwing with the build. Had to get rid of it. Best to either delete or put it in the .gitignore.

@musm
Copy link
Member

musm commented Sep 12, 2019

Also this is awesome! I always hated the libexpat shipped dependency just to bootstrap Pkg

@SimonDanisch
Copy link
Member

Btw, I just recently run into search still being used in here:

return !(isempty(search(a, b)))::Bool

That got removed from julia in 1.0... Not sure if we want to fix it here in this PR ;)

@SimonDanisch
Copy link
Member

I'm going to merge this if nobody is against it ;)

Project.toml Outdated
@@ -0,0 +1,16 @@
name = "LibExpat"
uuid = "522f3ed2-3f36-55e3-b6df-e94fee9b0c07"
version = "0.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

is it ok to set the version to this number?

Copy link
Member

Choose a reason for hiding this comment

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

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 you mean 0.5.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although maybe we should bump the version since this could be considered a big change.

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'll let you guys bump the version number if you want afterwards.

@musm musm merged commit 6933aba into master Sep 12, 2019
@SimonDanisch SimonDanisch deleted the sf/artifact branch September 12, 2019 22:26
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.

3 participants