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

Allow specifying the dev directory #866

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mortenpi
Copy link
Contributor

So that you could deploy the jll package to somewhere other than just the the global ~/.julia/dev directory, by passing --devdir=... on the command line. Related to #861 in that I find it useful when I work locally.

If this looks reasonable, I'd be happy to look into adding tests and docs. Or if you want to steal it for #861, you're also more than welcome 😛

@staticfloat
Copy link
Member

How about we combine this with --deploy? We can have --deploy=/absolute/path/to/directory deploy the code into that directory instead of using Pkg.devdir() automatically. We can disambiguate the user/repo case from this one by only triggering the GitHub-upload path if the given path is not an absolute path. We should of course pass it through expanduser() first just in case, so that users can provide paths such as ~/src or whatever.

Thoughts?

@mortenpi
Copy link
Contributor Author

We can disambiguate the user/repo case from this one by only triggering the GitHub-upload path if the given path is not an absolute path.

In my case I am actually setting it to a relative path (--devdir=dev; I just want to put the _jll into a dev directory that's next to the build_tarballs.jl script), so that heuristic wouldn't be ideal for me (of course, I could just do --deploy=$PWD/dev).

Also, aren't the dev. dir / code_dir and whether you deploy locally or to GitHub slightly orthogonal to each other? Because you may want to control where the (temporary) repository gets written to even if you are pushing to GitHub? I actually find it slightly surprising that BB would write into the global .julia/dev directory by default. Is there anything that precludes using a temporary directory (at least in principle)?

We should of course pass it through expanduser() first just in case, so that users can provide paths such as ~/src or whatever.

Ah, yes indeed.

@giordano
Copy link
Member

Also, aren't the dev. dir / code_dir and whether you deploy locally or to GitHub slightly orthogonal to each other? Because you may want to control where the (temporary) repository gets written to even if you are pushing to GitHub?

Yes, I agree, I was going to offer this counter-argument.

I actually find it slightly surprising that BB would write into the global .julia/dev directory by default. Is there anything that precludes using a temporary directory (at least in principle)?

I don't think there is any specific reason apart from convenience. Actually, writing to a temporary directory would make the test for the JLL packages a little bit easier and not polluting the global Pkg.devdir(), so I'm very much in favour of this feature!

@staticfloat
Copy link
Member

No, no reason not to use a temporary directory. I think the reason it used the dev directory by default is defunct now; there were some kind of efficiency concerns a while back, but those concerns are all gone now. Let's just switch to using a temporary directory by default.

I think the --deploy and --register options are a bit of a mess right now; we have --deploy-jll, --deploy-bin, --register, etc... and the logic is all quite confusing. Let's separate the concerns:

  • Writing JLL package to disk; note that right now, this only happens if we're going to be deploying the package somewhere (e.g. --deploy or --deploy=local).

  • Writing tarballs to disk; this always happens.

  • Deploying JLL package to GitHub; this only happens if --deploy/--deploy-jll is set appropriately.

  • Deploying tarballs to GitHub; this only happens if --deploy/--deploy-bin is set appropriately.

  • Registering the JLL wrapper; this only happens if --register is set appropriately.

IMO, we should make this as consistent as possible; right now, binaries are always created and are plopped into the current directory, JLLs are created only sometimes, and are plopped into ~/.julia/dev. I think we should make this consistent and allow setting of output directories for both, as well as setting of deployment targets for both.

Proposed new flags:

  • --output=<dir>; writes tarballs to <dir>/products and JLL package to <dir>/$name. Example: --output=/tmp/out will create /tmp/out/products/$tarball_name.tar.gz, and /tmp/out/LibFoo_jll/src/LibFoo_jll.jl. Setting each individually is possible through --output-jll=<dir> and --output-bin=<dir>, (setting --output sets both). Without anything specified, they default to pwd().

  • --deploy=username/repo; does what it does now, except that JLL generation is not contingent on --deploy being set; this means that JLL packages will now be written to pwd() by default if users run build_tarballs.jl.

  • --register; no changes, although we may want to allow specification of where to store the local registry used to build the PR; right now it stores it in ~/.julia/registries_binarybuilder, which is hardly optimal. I'm thinking this can probably be shunted off to a scratch space in Julia 1.6.

@mortenpi
Copy link
Contributor Author

Is --output-bin the products/ directory?

Also, this is possibly a very silly question: is there also a step where the artifacts get copied from products/ into the top depot? I am actually not 100% sure how, if I deploy locally and then do dev ~/path/to/my_jll, it finds the artifacts themselves (since they haven't been pushed to GitHub).

@staticfloat
Copy link
Member

Is --output-bin the products/ directory?

Yes, my proposal is that --output=<dir> is equivalent to --output-jll=<dir>/$(name)_jll.jl --output-bin=<dir>/products.

is there also a step where the artifacts get copied from products/ into the top depot?

Hilariously, it's actually the other way around:

  • First, autobuild() creates temporary build locations within build/$(triplet)/$(nonce) and does all of its business within there.
  • Once the build is done, there are a bunch of directories like srcdir and destdir and meta within the temporary build location, and destdir gets copied into a new artifact.
  • Once the artifacts are created, (which also calculates their treehash and whatnot) they get archived into tarballs within the products directory.

This means that everything that is a tarball in your products directory is already an artifact on your system that will get GC'ed after a while, unless you install a package that uses it.

@mortenpi
Copy link
Contributor Author

mortenpi commented Nov 18, 2020

I updated the PR to the --output* design. I didn't touch the --deploy yet, so I think --deploy=local is still necessary when working locally, and I am still defaulting to ~/.julia/dev for --output-jll, which is consistent with the current default behavior. So I believe at the moment the PR doesn't change any default behavior, but you can now specify the output directories if you want. If this broadly looks OK, I can do another iteration with tests and docs.

out_path = joinpath(dir, "products")
try mkpath(out_path) catch; end
# Our build products will go into products_dir (usually ./products)
try mkpath(products_dir) catch; end
Copy link
Member

Choose a reason for hiding this comment

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

I know you didn't introduce this line, but I don't think we want to wrap mkpath with a try/catch block 🤔

Copy link
Contributor Author

@mortenpi mortenpi Nov 22, 2020

Choose a reason for hiding this comment

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

Going back in Git blame, it looks like it used to be a very common pattern 3 years ago, but it looks like this is the last remaining relic. I can remove the try ... catch.

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