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

Switch to .toml files for new Pkg3 Registry process #202

Merged
merged 7 commits into from
Jul 23, 2019

Conversation

NHDaly
Copy link
Collaborator

@NHDaly NHDaly commented Jun 19, 2019

  • Add Project.toml, Manifest.toml
  • Remove REQUIRE

I directly converted our version specifications from the REQUIRE file (ie kept them as their corresponding >= meaning), giving us:
https://github.com/NHDaly/Blink.jl/blob/2c6e94d392b13cf20c6a0f025d31e57b187c9776/Project.toml#L22-L26

But I think more likely than not, we probably want to switch these to be semver versions. But I guess we should reflect first on whether those versions are even up to date anymore? I vaguely recall wanting to set some upper bound version requirements in the past and not being able to, but I don't think we have any such problems right now.
Any thoughts on what's best for this?

Thanks!

  • This is a reminder that after we merge this and start the registration PR, we should ping @travigd to start a registration PR for WebIO.jl.

- Add Project.toml, Manifest.toml
- Remove REQUIRE

Directly converted our version specifications from the REQUIRE file (ie
kept them as their corresponding `>=` meaning.
@NHDaly NHDaly requested a review from pfitzseb June 19, 2019 20:31
@NHDaly
Copy link
Collaborator Author

NHDaly commented Jun 19, 2019

Oh and also, @pfitzseb, I think I remember that you are in the anti-manifest file camp? I think I am in the opposed group as well, since it will hide breakages in upstream packages from our CI unless we keep it up-to-date. I think I vote for removing it, but i wanted to upload it here just for posterity. :)

@twavv
Copy link
Member

twavv commented Jun 20, 2019

You'll need to change the TravisCI file.

script:
  - $TESTCMD -e 'using Pkg; Pkg.build(); Pkg.test("Blink"; coverage=true)'

Manifest should be removed imo (it will just create git diff noise and serve no purpose).

@twavv
Copy link
Member

twavv commented Jun 20, 2019

WebIO PR: JuliaRegistries/General#1473

Copy link
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!
Left a couple of proposed version bounds as inline comments. Should also get rid of the manifest, and implement Travis' travis fix. :)

Project.toml Outdated Show resolved Hide resolved
Manifest.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@NHDaly
Copy link
Collaborator Author

NHDaly commented Jul 1, 2019

You'll need to change the TravisCI file.

script:
  - $TESTCMD -e 'using Pkg; Pkg.build(); Pkg.test("Blink"; coverage=true)'

Ah, thanks, good call! :) I've also changed the travis command for our docs build, which will hopefully work correctly as well.

Manifest should be removed imo (it will just create git diff noise and serve no purpose).

👍 I removed the Manifest file as well. Thanks!

@NHDaly
Copy link
Collaborator Author

NHDaly commented Jul 1, 2019

@travigd actually, unfortunately it looks like the latest WebIO v0.8.6 has broken Blink.

Can that version be fixed and maybe you can bump the breaking changes to be v0.9 so that we can keep an upper bound on [v0.3 - v0.8] and still work?
Here's the log showing the breakage:

(Blink) pkg> test Blink
     ...
   Testing Blink tests passed

(Blink) pkg> update
  Updating registry at `~/.julia/registries/General`
  Updating git-repo `https://github.com/JuliaRegistries/General.git`
 Resolving package versions...
 Installed WebIO ─ v0.8.6
  Updating `~/.julia/dev/Blink/Project.toml`
  [0f1e0344]  WebIO v0.8.5  v0.8.6
  Updating `~/.julia/dev/Blink/Manifest.toml`
  [cd3eb016]  HTTP v0.8.2  v0.8.3
  [0f1e0344]  WebIO v0.8.5  v0.8.6
  Building WebIO  `~/.julia/packages/WebIO/9qsL8/deps/build.log`

(Blink) pkg> build
  Building MbedTLS  `~/.julia/packages/MbedTLS/X4xar/deps/build.log`
  Building WebIO ── `~/.julia/packages/WebIO/9qsL8/deps/build.log`
  Building Blink ── `~/.julia/dev/Blink/deps/build.log`
 Resolving package versions...

(Blink) pkg> test Blink
   Testing Blink
 Resolving package versions...
ERROR: LoadError: Javascript error      Error: Cannot find module 'debug'

@NHDaly
Copy link
Collaborator Author

NHDaly commented Jul 2, 2019

(and to clarify, i get the same issues even if I pin HTTP back to v0.8.5, so I think the issue is with WebIO):

(Blink) pkg> test Blink
   Testing Blink
 Resolving package versions...
ERROR: LoadError: Javascript error      Error: Cannot find module 'debug'
...
ERROR: Package Blink errored during testing

(Blink) pkg> status
Project Blink v0.11.0
    Status `~/.julia/dev/Blink/Project.toml`
...
  [cd3eb016] + HTTP v0.8.2 #6748814 (https://github.com/JuliaWeb/HTTP.jl.git) ⚲
...
  [0f1e0344] + WebIO v0.8.6
...

@NHDaly
Copy link
Collaborator Author

NHDaly commented Jul 2, 2019

Cool! Sorry this got dropped for two weeks.

I was hoping to finish it before I went on vacation all of last week, but didn't manage it.

Blink is now broken on master from the latest WebIO change; I've opened an issue about it here: JuliaGizmos/WebIO.jl#317
Pending that fix, this PR should be good to go! :)

In the meantime I guess we could also change our compatability to more specifically upper-bound against v0.8.6?
Or maybe set 0.7 as our upper-bound for this PR and then add a new release that adds 0.8 support?

WebIO v0.8.6 broke Blink, so we're downgrading to 0.7 for now. More info
on this issue, here: JuliaGizmos/WebIO.jl#317
@NHDaly
Copy link
Collaborator Author

NHDaly commented Jul 22, 2019

Okay, I've just gone ahead and downgraded WebIO to "0.7" for now. Assuming that seems reasonable, this should be good to merge once the CI build succeeds. :)

@NHDaly NHDaly force-pushed the register-Pkg3-Registry branch from 31216ac to e0ecc45 Compare July 22, 2019 19:32
@pfitzseb
Copy link
Member

Alright, everything's green. Wanna merge + tag? :)

@codecov-io
Copy link

codecov-io commented Jul 22, 2019

Codecov Report

Merging #202 into master will increase coverage by 4.53%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
+ Coverage   41.09%   45.63%   +4.53%     
==========================================
  Files          10       12       +2     
  Lines         292      309      +17     
==========================================
+ Hits          120      141      +21     
+ Misses        172      168       -4
Impacted Files Coverage Δ
src/AtomShell/AtomShell.jl 100% <0%> (ø)
src/Blink.jl 100% <0%> (ø)
src/content/server.jl 77.5% <0%> (+0.57%) ⬆️
src/rpc/callbacks.jl 92.3% <0%> (+1.39%) ⬆️
src/rpc/rpc.jl 72.22% <0%> (+3.47%) ⬆️
src/AtomShell/process.jl 73.8% <0%> (+3.8%) ⬆️
src/AtomShell/window.jl 36.98% <0%> (+6.55%) ⬆️
src/content/content.jl 88.88% <0%> (+10.31%) ⬆️

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 ee0f64d...e0ecc45. Read the comment docs.

@NHDaly NHDaly merged commit 77b729a into JuliaGizmos:master Jul 23, 2019
@NHDaly NHDaly deleted the register-Pkg3-Registry branch July 24, 2019 03:41
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.

4 participants