search

ScenicFramework / scenic_driver_glfw

Scenic render and input driver for windowed OSs
Apache License 2.0
28 stars 21 forks source link

Add mix.lock file #27

Open axelson opened 5 years ago

axelson commented 5 years ago

Add a mix.lock file with the latest version of depenedencies. Tests run fine with these for me and they all match mix.exs.

Will help bugs arising from contributors using different versions of dependencies. Similar to what happened in: https://github.com/boydm/scenic_driver_glfw/pull/26

Alternatively we may want to add this is #26 directly in which case this can just be closed (also I figured that we'd bump the elixir_make version in #26 since it is semi-related)

Eiji7 commented 5 years ago

Do we or should we have a test for mix deps.update --all task i.e. checking if mix.lock is changed?

boydm commented 5 years ago

@axelson @Eiji7 Thinking about this. I think the root problem with the other PR was that the mix deps was simply on an old version. Wasn't really about the mix.lock file.

What are the downsides (if any??) of including the lock file?

Eiji7 commented 5 years ago

@boydm I just found an amazing resource for us: https://elixirforum.com/t/of-course-you-should-version-mix-lock-wait-do-we/14910?u=eiji

TLDR;

We should:

  1. Add mix.lock file
  2. Optionally add CI command to remove mix.lock before compile (to ensure this project works with latest dependencies)
boydm commented 5 years ago

@Eiji7 Interesting. And reminds me that the reason I didn't include the lock files was to allow the packages to "float" version-wise in the host project. Use its dependencies as it were.

I'll re-read it again tomorrow.

axelson commented 5 years ago

I agree that the approach of allowing the dependencies to "float" in CI by removing the mix.lock before running mix deps.get would work well, both for contributors staying on the same deps, as well as testing the latest versions of dependencies. Although another option is the (now free) dependabot service which will automatically create a new PR to update to the latest version of any new dependency. And that PR of course would trigger CI so it could be tested if it breaks the build.

boydm commented 5 years ago

@axelson @Eiji7 So... Is there a consensus on how to handle the CI? One of you guys want to add the script to remove the mix.lock?

Eiji7 commented 5 years ago

@boydm I believe that this solution + @axelson proposition would be really helpful. However something is still in my mind … Should we prevent updating mix.lock by others than dependabot? I have not tested it before, so I'm not sure if it's even possible …

I'm not sure if I would have time soon and if so I would like to take a look at UI scaling issue. Maybe I could debug it somehow and give you any ideas on it … I did not yet analyzed whole code, so it would be helpful if you could give me a hint where scaling on window resize is handled.

boydm commented 5 years ago

@eiji7 Right. Still doesn't quite sit right. In fact I think the core issue we were facing in the first place was that it was the wrong version elixir_make. Or rather, that elixir_make made a breaking change that affected the driver. Including the mix.lock file might be a "red herring" that could cause more trouble than it solves.

axelson commented 5 years ago

I am definitely still in favor of having a mix.lock committed in the repository and I am of the opinion that it will be more helpful than hurtful. I don't think it needs to be locked down to only dependabot because sometimes as a contributor you will want to update the version requirement in mix.exs and mix.lock simultaneously.

I think the CI approach I am in favor of is to always run CI against the upper-most versions specified in mix.exs which means that the CI will be operating effectively as if the mix.lock file was not committed to the repository. In addition to running CI on every pull request/branch push I think it would be advantageous to set the CI to run every day (using the cron feature: https://docs.travis-ci.com/user/cron-jobs/), that way if there's a new release of a downstream library that breaks scenic_driver_glfw, we can be notified of it right away. Dependabot could then be added on top of this setup as a developer nicety to not need to manually keep mix.lock up to date.

I'm working on getting a branch with a working travis configuration up as a base, but I'm running into a little trouble. Possibly because the CI is run on a headless server, I haven't dug into it enough yet (you can find my work in progress at https://github.com/axelson/scenic_driver_glfw/tree/add-travis-config and an example failing build is at https://travis-ci.org/axelson/scenic_driver_glfw/jobs/564751545). I am also able to reproduce the build failure on my headless ubuntu server (running 19.04) which should help in figuring out the fix.