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

If user does e.g. Pkg.add("Foo.jl"), instruct them to do Pkg.add("Foo") instead #2099

Merged
merged 1 commit into from Nov 26, 2020
Merged

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Oct 11, 2020

In documentation and other prose, we often refer to Julia packages as "PackageName.jl". So I think it is understandable that a user would try to do Pkg.add("PackageName.jl").

This PR prints an instructive message if a user tries to do that.

@fredrikekre
Copy link
Member

I think this should be part of the error message instead, e.g.

julia> Pkg.add("Example.jl")
ERROR: `Example.jl` is not a valid package name, perhaps you meant `Example`?

instead of the current

julia> Pkg.add("Example.jl")
ERROR: `Example.jl` is not a valid package name

@DilumAluthge
Copy link
Member Author

Done.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Oct 12, 2020

I think the CI failures are unrelated: (click to expand)
411precompile: Error During Test at /home/travis/build/JuliaLang/Pkg.jl/test/new.jl:1819
412  Got exception outside of a @test
413  MethodError: no method matching Base.TOMLCache()
414  Closest candidates are:
415    Base.TOMLCache(::Base.TOML.Parser, ::Dict{String, Base.CachedTOMLDict}) at loading.jl:215
416    Base.TOMLCache(::Any, ::Any) at loading.jl:215
417  Stacktrace:
418    [1] precompile(ctx::Pkg.Types.Context; internal_call::Bool)
419      @ Pkg.API ~/build/JuliaLang/Pkg.jl/src/API.jl:968
420    [2] precompile(; internal_call::Bool)
421      @ Pkg.API ~/build/JuliaLang/Pkg.jl/src/API.jl:916
422    [3] precompile()
423      @ Pkg.API ~/build/JuliaLang/Pkg.jl/src/API.jl:916
424    [4] (::Main.PkgTests.NewTests.var"#544#546")()
425      @ Main.PkgTests.NewTests ~/build/JuliaLang/Pkg.jl/test/new.jl:1829
426    [5] (::Main.PkgTests.Utils.var"#4#7"{Bool, Main.PkgTests.NewTests.var"#544#546"})()
427      @ Main.PkgTests.Utils ~/build/JuliaLang/Pkg.jl/test/utils.jl:52
428    [6] withenv(::Main.PkgTests.Utils.var"#4#7"{Bool, Main.PkgTests.NewTests.var"#544#546"}, ::Pair{String, Nothing}, ::Vararg{Pair{String, Nothing}, N} where N)
429      @ Base ./env.jl:161
430    [7] isolate(fn::Main.PkgTests.NewTests.var"#544#546"; loaded_depot::Bool)
431      @ Main.PkgTests.Utils ~/build/JuliaLang/Pkg.jl/test/utils.jl:43
432    [8] isolate(fn::Function)
433      @ Main.PkgTests.Utils ~/build/JuliaLang/Pkg.jl/test/utils.jl:18
434    [9] top-level scope
435      @ ~/build/JuliaLang/Pkg.jl/test/new.jl:1828
436   [10] top-level scope
437      @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1144
438   [11] top-level scope
439      @ ~/build/JuliaLang/Pkg.jl/test/new.jl:1821
440   [12] include(mod::Module, _path::String)
441      @ Base ./Base.jl:389
442   [13] include(x::String)
443      @ Main.PkgTests ~/build/JuliaLang/Pkg.jl/test/runtests.jl:3
444   [14] top-level scope
445      @ ~/build/JuliaLang/Pkg.jl/test/runtests.jl:16
446   [15] include(fname::String)
447      @ Base.MainInclude ./client.jl:444
448   [16] top-level scope
449      @ none:6
450   [17] eval(m::Module, e::Any)
451      @ Core ./boot.jl:360
452   [18] exec_options(opts::Base.JLOptions)
453      @ Base ./client.jl:261
454   [19] _start()
455      @ Base ./client.jl:485
456Test Summary: | Pass  Error  Total
457precompile    |    2      1      3
458ERROR: LoadError: LoadError: Some tests did not pass: 2 passed, 0 failed, 1 errored, 0 broken.
459in expression starting at /home/travis/build/JuliaLang/Pkg.jl/test/new.jl:1
460in expression starting at /home/travis/build/JuliaLang/Pkg.jl/test/runtests.jl:3
461ERROR: Package Pkg errored during testing
462Stacktrace:
463 [1] pkgerror(msg::String)
464   @ Pkg.Types ~/build/JuliaLang/Pkg.jl/src/Types.jl:52
465 [2] test(ctx::Pkg.Types.Context, pkgs::Vector{Pkg.Types.PackageSpec}; coverage::Bool, julia_args::Cmd, test_args::Cmd, test_fn::Nothing)
466   @ Pkg.Operations ~/build/JuliaLang/Pkg.jl/src/Operations.jl:1632
467 [3] test(ctx::Pkg.Types.Context, pkgs::Vector{Pkg.Types.PackageSpec}; coverage::Bool, test_fn::Nothing, julia_args::Cmd, test_args::Cmd, kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
468   @ Pkg.API ~/build/JuliaLang/Pkg.jl/src/API.jl:334
469 [4] #test#62
470   @ ~/build/JuliaLang/Pkg.jl/src/API.jl:71 [inlined]
471 [5] test(; name::Nothing, uuid::Nothing, version::Nothing, url::Nothing, rev::Nothing, path::Nothing, mode::Pkg.Types.PackageMode, subdir::Nothing, kwargs::Base.Iterators.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:coverage,), Tuple{Bool}}})
472   @ Pkg.API ~/build/JuliaLang/Pkg.jl/src/API.jl:87
473 [6] top-level scope
474   @ none:1
475The command "julia --project --check-bounds=yes -e 'import Pkg; Pkg.build(); Pkg.test(; coverage=true)'" exited with 1.
476
477
478Done. Your build exited with 1.

@DilumAluthge
Copy link
Member Author

@ianshmean Will the CI errors that I'm seeing in this PR will be fixed by #2091?

@IanButterworth
Copy link
Sponsor Member

Indeed. #2091 also includes the fix from #2102 and an additional related fix

@DilumAluthge DilumAluthge marked this pull request as draft October 12, 2020 04:02
@DilumAluthge DilumAluthge deleted the patch-1 branch October 12, 2020 04:02
@DilumAluthge DilumAluthge restored the patch-1 branch October 12, 2020 04:04
@DilumAluthge DilumAluthge reopened this Oct 12, 2020
@DilumAluthge DilumAluthge marked this pull request as ready for review October 12, 2020 04:07
@DilumAluthge
Copy link
Member Author

Alright, this is good to go assuming CI passes.

@DilumAluthge
Copy link
Member Author

Hmmmm. @ianshmean Any chance you know what's causing these new CI failures?

@IanButterworth
Copy link
Sponsor Member

I think this needs a rebase

@DilumAluthge
Copy link
Member Author

Alright, looks like tests are finally passing.

@DilumAluthge
Copy link
Member Author

@fredrikekre Is this good to merge now?

@fredrikekre
Copy link
Member

Maybe add a test?

@DilumAluthge
Copy link
Member Author

Maybe add a test?

Good idea. Done.

@DilumAluthge
Copy link
Member Author

Bump.

test/api.jl Outdated Show resolved Hide resolved
@DilumAluthge
Copy link
Member Author

CI failures are due to Pkg server woes.

@KristofferC
Copy link
Sponsor Member

Think you need to be a bit careful with string indexing, for example:

julia> x = "α.jl"
"α.jl"

julia> message = "`$x` is not a valid package name"
"`α.jl` is not a valid package name"

julia> if endswith(lowercase(x), ".jl")
           message *= ". Perhaps you meant `$(x[1:(end-3)])`"
       end
ERROR: StringIndexError("α.jl", 2)
Stacktrace:
 [1] string_index_err(::String, ::Int64) at ./strings/string.jl:12
 [2] getindex(::String, ::UnitRange{Int64}) at ./strings/string.jl:250
 [3] top-level scope at REPL[4]:2

@StefanKarpinski
Copy link
Sponsor Member

Using chop(; tail=3) should be safe. I'm still unclear why we don't just do the right operation instead of lecturing the user in this case. It's not like Foo.jl is a valid package name anyway.

@KristofferC
Copy link
Sponsor Member

I think we do that in the repl.

@StefanKarpinski
Copy link
Sponsor Member

Fair enough. Makes sense to be stricter in the API.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Nov 21, 2020

Good catch on the string indexing. I've changed it to use chop(x; tail=3).

@DilumAluthge
Copy link
Member Author

I think we do that in the repl.

This is correct.

(@v1.5) pkg> add Example.jl
  Resolving package versions...
  Installed Example ─ v0.5.3
Updating `~/.julia/environments/v1.5/Project.toml`
  [7876af07] + Example v0.5.3
Updating `~/.julia/environments/v1.5/Manifest.toml`
  [7876af07] + Example v0.5.3

Makes sense to be stricter in the API.

Agreed.

@DilumAluthge DilumAluthge reopened this Nov 22, 2020
@fredrikekre fredrikekre merged commit 46eb2b2 into JuliaLang:master Nov 26, 2020
@DilumAluthge DilumAluthge deleted the patch-1 branch November 26, 2020 14:24
@IanButterworth IanButterworth added the backport 1.6 Change should be backported to release-1.6 label Mar 7, 2021
IanButterworth pushed a commit to IanButterworth/Pkg.jl that referenced this pull request Mar 7, 2021
@IanButterworth IanButterworth mentioned this pull request Mar 26, 2021
14 tasks
@fredrikekre fredrikekre removed the backport 1.6 Change should be backported to release-1.6 label Jun 4, 2021
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

5 participants