litex icon indicating copy to clipboard operation
litex copied to clipboard

Version control with JSON file

Open kaolpr opened this issue 3 years ago • 12 comments

When debugging https://github.com/enjoy-digital/litedram/issues/293 I found it difficult to reproduce a complete LiteX environment with all dependencies in a specified repository revision. LiteX setup script by default pulls latest master, so installation leads to a different result at different times.

This PR attempts to address this issue by introducing a new option for litex_setup.py: --json JSON. It enables an overlay for defined source repositories. Such overlay can overwrite any GitRepo parameter (or create a new repository entry) what can be used to define a specific SHA1 for repository.

It also introduces a new tool - litex_freeze that can create JSON overlay out of existing LiteX installation directory.

To make overlays more consistent and straightforward while keeping implementation minimal, I've decided to change way repo URL is expressed in git_repos. Now key is name of target directory (where sources are cloned to) and GitRepo constructor is given a full repo URL.

kaolpr avatar Feb 23 '22 23:02 kaolpr

FYI - This is starting to replicate what Python's requirements.txt and tools like pip-lock are designed to do (and conda environment.yml files and conda-lock too). A number of projects are also using git submodules to do this type of thing too.

mithro avatar Feb 24 '22 03:02 mithro

I'm aware of that and honestly I'd very much prefer having all repositories packed into Python packages and let user decide which of them are needed.

However, this would require quite a work both in implementation (define CI for every project) and in defining rules of what is in master, when a new version is defined etc.

I wanted to craft a solution that could be applied quickly and in non intrusive way into existing installation architecture.

kaolpr avatar Feb 24 '22 08:02 kaolpr

Thanks @kaolpr, I also had a need for that for different projects and was going to look at implementing this. I'll have a closer look at your implementation very soon.

@mithro: The litex_setup.py script is very simple and (I think) gives more flexibility for the development. We could however think about using existing lock mechanisms for the regular releases.

enjoy-digital avatar Feb 24 '22 09:02 enjoy-digital

@enjoy-digital is the BDFL for this project, so whatever he says goes.

It is a very common "anti-pattern" in open source development to create something that is "super simple" because you don't want or need the complexity of the more common solution and then to slowly reinvent (normally poorly) all the complexity found in the original solution you were originally trying to avoid. Every time litex_setup.py gets a little another feature and thus gets slightly more complicated I worry that we are repeating this common mistake.

Two other quick comments;

  • For the pythondata-XXX packages under LiteX-Hub we are automatically uploading to PyPi on every commit pushed to the master/main branch using GitHub Actions. We could most certainly set up the same thing for LiteX repos.
  • You can also specify a specific commit on requirements.txt or environment.yml line -- see https://pip.pypa.io/en/stable/topics/vcs-support/#git

mithro avatar Feb 24 '22 16:02 mithro

Building packages for every commit would indeed make transition to package-based solution conceptually way easier. And to be honest, I'd be very much in favor of pip-based approach.

If @enjoy-digital agrees for that direction, I'd be very happy to help.

kaolpr avatar Feb 24 '22 17:02 kaolpr

@mithro: There are two different things: Development and deployment. litex_setup.py has been created for my development and happens to be also currently used for deployment but this was not the original purpose.

For development, I have a preference to have a full control the tool I'm using (and I'm also using derivative version of litex_setup.py for private projects with clients with other private repositories), so I find litex_setup.py is convenient for the and I also wanted to introduce the features @kaolpr had a need for.

For deployment, I'm less regarding and would be fine having alternatives to litex_setup.py to allow full install of LiteX with pip, but that's not my expertise nor necessarily something I want to maintain. So I'm open to ideas on this, but don't necessarilly have the expertise/time to look at this and litex_setup.py would still exist for different purposes.

enjoy-digital avatar Feb 24 '22 17:02 enjoy-digital

@enjoy-digital This functional breakdown is pretty convincing to me. Though pip is also widely used in development, and I personally prefer sticking to well-established solutions, I'm also fine with having custom setup script approach. As long as it enables some degree of reproducibility. So, I still submit this PR for review ;-).

When it comes to deployment, I can create another issue to clarify the way use of pip packages could be implemented and get involved with the implementation when idea settles.

kaolpr avatar Feb 24 '22 20:02 kaolpr

@kaolpr: Sure, I'll review it. I also had some ideas on my side and while adding this feature I want to be sure it will cover my needs on private project, so will plan to time to work on this probably next week.

enjoy-digital avatar Feb 24 '22 21:02 enjoy-digital

I want all my end users (who are doing deployment) to become developers :-).

mithro avatar Feb 24 '22 21:02 mithro

@enjoy-digital What's status on reproducibility? Do you have any new thoughts on that matter?

kaolpr avatar Nov 10 '22 15:11 kaolpr

@kaolpr: Sorry for the lack of update, so a --freeze command has been added to litex_setup.py , this generate a git_repos dict that can be re-integrated in a custom litex_setup.py used for deployment. I now need to add a function to re-import this directly and avoid manual modification of litex_setup.py for deployement.

enjoy-digital avatar Nov 10 '22 15:11 enjoy-digital

@enjoy-digital Why did you choose to use Python output instead of JSON? I believe it would be more handy for CI...

kaolpr avatar Jan 05 '23 13:01 kaolpr