TestEnv.jl icon indicating copy to clipboard operation
TestEnv.jl copied to clipboard

Use conditionals in code instead of different code on different branches

Open davidanthoff opened this issue 4 years ago • 8 comments

I would love to use this in the VS Code extension, but the current model of having separate branches for different Julia versions won't work for us: we can only ship one version of each package, and that version needs to just work on all supported Julia versions.

Couldn't this entire package just be done as a normal package, and the TestEvn.jl file looks like:

module TestEnv

@static if VERSION==something
    include("julia_1_1.jl")
elseif VERSION == somethingelse
  include("julia_1.2.jl"
end

end

As far as I can tell that would also allow us to have completely different code-bases for different Julia versions, but at the same time the package would look a lot more like a "normal" package? Plus, we could use it :)

davidanthoff avatar Aug 09 '21 18:08 davidanthoff

I would love for this to be usable for VS-Code. There are two problems with packaging it normally like you say.

  1. Test dependencies: for testing we try this out on real packages. And so to check that this works with packages with a test/Project.toml we have a test dependency on a package that uses that, but such a package by its nature only supports julia 1.2+ when that feature was added

    • possible resolution 1) workout how to test this with fake packages that are just directories. I tried and it did my head in trying to workout how to have them seen as environments
    • possible resolution 2) install the packages at test time into a temp environment, and each of the code for installing the packages could be gated behind if VERSION
  2. porting code to difference versions: right now moving code to correct bugs or add features to different versions is relatively straight forward. Fix it on any one branch, then git cherrypick the code on to the others that need it. (and so far for the 1 bug I fixed it seems like when the bug occurs in multiple versions that implies the code is similar enough for cherrypick to find where it needs to go)

    • possible resolution: I think unix diff and patch can do the same thing. But idk how to do it.

I assume the reason VS-code can't support this is because it makes a git submodule for each of it's dependencies? Is there a reason why it couldn't do a bunch of submodules one per release branch?

oxinabox avatar Aug 09 '21 19:08 oxinabox

Following up from #34

@KristofferC said:

As it is right now, it is pretty much impossible to get a view of what different codes there are and how they differ between versions.

Use standard git (/github) tools to compare between branches. It is no more or less easy than comparing between two files. It's just git diff instead of diff. (alt you can use githubs diff tool)

oxinabox avatar Nov 23 '21 13:11 oxinabox

@davidanthoff one idea for how to support VS-Code is we could generate a mono-branch in a GitHubAction. That would check out each branch and combine their codes like you say.

Then we get all the advantages of branches for ease of porting code + ease of testing. But still end up with sometime you can use.

oxinabox avatar Jan 20 '22 18:01 oxinabox

Ok, I'm back working on testing in VS Code (see https://github.com/julia-vscode/julia-vscode/pull/2350 for a snapshot) and we will need something like this package here to bring that over the finishing line.

I have to admit that I'd still love to see this package transformed into a "standard" format :) I'm just super wary of taking dependencies on stuff that uses clever non-standard ways of doing things. In my experience those choices never really end well in the long run: typically, only the original package author ever really understands things, if I run into a problem on our end and quickly want to fix something, I now need to spend significant amount of time trying to understand what is going on, instead of just being able to do a quick PR etc. Plus, with the VS Code extension in particular, we already have such a complicated setup overall, that I don't want to add dependencies that require contributors to understand even more esoteric choices. I think @KristofferC's reaction is an example of what I mean: he is probably one of the most knowledgeable folks on package manager questions, in my ideal world this package here would be in a state that he can take a quick look at something and help, rather than leaving him confused about the basic structure of things :)

A mono-branch generated from a GitHub Action to me sounds like going further down the rabbit hole of something that is simply too complicated and non-standard.

On the two points you raise: for the testing stuff, I really like your solution 2) "install the packages at test time into a temp environment". On the porting of patches, is the current setup really less work? To me it sounds like an awful lot of git steps one needs to do if there is a simple code change... But I'm not sure about that.

davidanthoff avatar Mar 24 '22 19:03 davidanthoff

Sorry for the slow reply, I got covid.

A mono-branch generated from a GitHub Action to me sounds like going further down the rabbit hole of something that is simply too complicated and non-standard.

Fair point.


Ok once i get to the bottom of my backlog of code to review I will look at what it looks like to turn this into single trunk repo. And make that branch.

oxinabox avatar Apr 12 '22 05:04 oxinabox

@oxinabox Alrighty, I'm at a point where it would be great if we could try to pull this off :) I'd also be happy to help, just not sure what the best strategy is. I think one key question is whether you want to preserve history, or whether we can just create a new main branch and then manually copy all the content from the existing branches into subfolders and have one big commit that adds everything? There wouldn't be history, but it would be a heck of a lot easier ;)

davidanthoff avatar Aug 14 '22 02:08 davidanthoff

@oxinabox I started doing the quick and dirty version that doesn't preserve history, take a look at https://github.com/davidanthoff/TestEnv.jl/tree/main. For now I'm using that fork/branch in the VS Code extension, so that essentially has the structure that works for us.

That fork does not handle testing at all yet... But for now it unblocks progress on my end.

davidanthoff avatar Aug 14 '22 22:08 davidanthoff

I do not feel the urge to preserve history. I do feel the urge to preserve tests though.

oxinabox avatar Aug 31 '22 13:08 oxinabox

As a compromise to deal with the Julia <= 1.2 vs Julia >= 1.2 split: one could also keep the multi branch model but reduced to two branches: one for Julia <= 1.2 and one for Julia >= 1.3. Then VSCode could just use the version for Julia >= 1.3, that would surely cover the vast majority of users?

I also had a look at the difference between the various branches and was surprised how little the differences were in practice, at least when comparing the branches for Julia 1.6 and upwards. Given that 1.6 is the current LTS, one could also split the package at that point.

I'll submit a few PRs that just reduce artificial diffs between the branches.

fingolfin avatar Jun 02 '23 14:06 fingolfin

So, for the VS Code extension https://github.com/davidanthoff/TestEnv.jl/tree/main is the way to go. Reducing the number of branches to 2 won't work for us. Also, from the VS Code side of things we need to support all Julia versions since 1.0, so splitting at 1.6 is also not an option for us.

I think if someone wants to help with this transition, the best strategy would be to open PRs against https://github.com/davidanthoff/TestEnv.jl/tree/main that add tests back in. And then once we have that branch will a full test suite, we can replace the situation here with that from my fork.

I think more work here in the origin to improve things will at the end of the day make this whole story more complicated, not less, just because we would then have to manually port those things over to my branch as well...

davidanthoff avatar Jun 12 '23 17:06 davidanthoff

That sounds good to me. I think a lot of the work for adding the tests back in is scripting work in the CI config to swap out different Project.toml files or otherwise to setup a good testing eviroment.

Its not something I have time or interest to do right now, but if someone is keen on it, it would definitely take us closer.

oxinabox avatar Jun 13 '23 09:06 oxinabox

I'll probably have a go at this, the longer I wait with this the more painful it will be to catch up with anything happening here :)

davidanthoff avatar Jun 13 '23 17:06 davidanthoff

Alright, I've now added back all the tests in my fork, and also expanded CI to run on Mac and Linux and x86. https://github.com/davidanthoff/TestEnv.jl/actions/runs/5260518893 shows the last test run where everything passes.

@oxinabox I think that means that you could just push the main branch from https://github.com/davidanthoff/TestEnv.jl to over here, make it the default branch and then we just need to tag a new release?

Couple of other things:

  • I've bumped the version to 2, but that might not be necessary. We could presumably also tag a v1.11 from that new main branch, and given that it is compatible with all Julia versions, that would then presumably become the version that everyone gets? That seems right to me, but someone else better also think this through.
  • I think the original idea was that src/common.jl would have the same content on all the version branches? That is not the case, so my version now has a separate copy of common.jl in each of the version folders. But down the road one could probably deduplicate a lot of code by only having one common.jl that actually has the code that is common between all Julia versions.
  • I didn't do anything for Julia v1.10. The repo here had a branch for that, but as far as I could tell there was nothing changed on it, so I don't think we need that with the new model. I guess we could add 1.10 to the CI, though.
  • I originally branched off this repo here in August 22. I now manually tried to port everything that was committed since then to my fork. I hope I got it all :)

I think from my end the highest priority would be that we try to transition this quickly now, or at least no merge anything new here in this original repo until we have finished this migration. I should have finished this all in one swoop last summer because that wait has made it quite a bit more complicated now, so hopefully we can avoid another of these delays where my fork falls behind :)

davidanthoff avatar Jun 13 '23 21:06 davidanthoff

This looks good to me from reviewing your main branch.

I've bumped the version to 2

I think I will bump to version 1.100.0 because this is nonbreaking. And that leaves plenty of space to go back to old versioning if we need to for some reason (I don't see why, but version numbers are cheap)

I think the original idea was that src/common.jl would have the same content on all the version branches?

No, that's the common code between activate_do.jl and activate_set.jl , which originally contained more code than they actually ended up with.

I didn't do anything for Julia v1.10. The repo here had a branch for that, but as far as I could tell there was nothing changed on it, so I don't think we need that with the new model.

Correct. We might need a folder for it if 1.10 does change a lot from 1.9 but at present that seems unlikely.

I will merge this shortly

oxinabox avatar Jun 14 '23 01:06 oxinabox

I think I will bump to version 1.100.0

Yep, perfect!

davidanthoff avatar Jun 14 '23 03:06 davidanthoff