Re-design of submodules used in blender.git #104755

Merged
Sergey Sharybin merged 12 commits from Sergey/blender:submodules_refactor into blender-v3.5-release 2023-02-21 16:40:14 +01:00

Basically, implementation of the #104573.

Combines changes to the repository itself, as well as changes to make update.
The make update also modified to support Github style of upstream remote configuration.

Basically, implementation of the #104573. Combines changes to the repository itself, as well as changes to `make update`. The `make update` also modified to support Github style of `upstream` remote configuration.
Sergey Sharybin added 2 commits 2023-02-14 18:34:11 +01:00
ec05e82b5c make_update: Support upstream remote workflow and update for subodules redesign
There are two aspects of the change which is a bit hard to decouple one
from another.

One part of it is to move away from submodules, as per #104573.

The change ensures that the blender-addons and blender-addons-contrib
repositories are cloned under scripts/addons and scripts/addons_contrib
respectively.

If there were configure d submodules for those repositories they are
copied over to the new location, and nothing else happens. If an upstream
workflow is desired it is to be configured manually. This ensures that
the local work is preserved as good as possible.

If there are no submodules configured then a new checkout will be done,
following the remote organization from the main repository: if there is
remote called "upstream" in the main repository, the same semantic will
be used for addons and addons_contrib.

NOTE: The submodules are never deinitialized in the local checkout, this
is to be done manually after ensuring there is no work which can be lost.

The other part of the change is to support workflow when "origin" points
to a fork, and "upstream" points to the upstream repository. This is rather
common workflow in all sort of forges like Github, Gitlab, perhaps even
Gitea. Having better support of our tools for it seems to have string
benefits.

The idea of this change is to make it so `make update` will perform the
steps needed to update the branch of the fork to the upstream version.

This includes:
- Fetch the `upstream` remote
- `git merge --ff-only upstream/branch` if the current branch exists in
  the upstream.
- If the branch does not exist in the upstream, then it will be pulled
  from the `origin` instead (the behavior prior to this change).

The same logic is implemented for the addons and addons_contrib.

The `make update` will also check for the forked addons and addons-contrib
within the same user/organization as the blender.git fork and add the
"origin" to the checkout of addons when such a fork is detected. This allows
to fork the addons repository after the `make update` has been run for the
first time.
bd8c1ebc05 Re-design of submodules used in blender.git
See #104573 for the full explanation.

Brief summary is:

- `addons` and `addons_contrib` are now expected to be a checkout of
  corresponding repositories inside of top-level `scripts` folder.

- `scripts` are moved from `release/scripts` to the top level.

- Developer tools and locales are embedded to the main repository,
Sergey Sharybin requested review from Brecht Van Lommel 2023-02-14 18:36:58 +01:00
Ray molenkamp reviewed 2023-02-14 19:07:00 +01:00
@ -964,2 +963,2 @@
"This is a 'git submodule', which are known not to work with bridges to other version "
"control systems."
"Translation path '${CMAKE_SOURCE_DIR}/locale' is missing. "
"This is an external repository which needs to be checked out. Use `make update` to do do. "
Member

to do do? -> to do so

to do do? -> to do so
brecht marked this conversation as resolved
Brecht Van Lommel added this to the Core project 2023-02-15 09:53:58 +01:00
Sebastian Parborg reviewed 2023-02-15 12:12:04 +01:00
@ -956,3 +956,3 @@
if(WITH_INTERNATIONAL)
file(GLOB RESULT "${CMAKE_SOURCE_DIR}/release/datafiles/locale")
file(GLOB RESULT "${CMAKE_SOURCE_DIR}/locale")

Is this check even needed anymore now that you have included all the locale files into the main repository?

I think this check should always succeed now if a standard git clone was successful.
Either way, the error message doesn't make sense anymore as it is not a external repository anymore.

Is this check even needed anymore now that you have included all the `locale` files into the main repository? I think this check should always succeed now if a standard git clone was successful. Either way, the error message doesn't make sense anymore as it is not a external repository anymore.
Author
Owner

Indeed this block can be removed.

Indeed this block can be removed.
Sergey marked this conversation as resolved
Sebastian Parborg reviewed 2023-02-15 12:58:09 +01:00
@ -14,3 +14,3 @@
- branch: main
commit_id: HEAD
path: release/datafiles/locale
path: datafiles/locale

I think we should scrub the repo of other locale related entries like these as well, right?

I think we should scrub the repo of other `locale` related entries like these as well, right?

This whole section could be removed as well as they are not submodules anymore.

This whole section could be removed as well as they are not `submodules` anymore.
Author
Owner

The locale and tools should be removed from the pipeline_config indeed. The rest need to stay as addons are not managed by the main repository.

Not sure there is more clear name that submodule, can rename it to externals but is also not that important. More imporant is to have the buildbot to support the new organization of folders.

I think we should scrub the repo of other locale related entries like these as well, right?

If you can spot the cases i've missed in the main repository you can as well make changes directly (the PR is marked for being editable by maintainers). The only missing case I am aware of is in the addons repository, which can not be included into this patch.

The `locale` and `tools` should be removed from the pipeline_config indeed. The rest need to stay as addons are not managed by the main repository. Not sure there is more clear name that `submodule`, can rename it to `externals` but is also not that important. More imporant is to have the buildbot to support the new organization of folders. > I think we should scrub the repo of other locale related entries like these as well, right? If you can spot the cases i've missed in the main repository you can as well make changes directly (the PR is marked for being editable by maintainers). The only missing case I am aware of is in the addons repository, which can not be included into this patch.
Sergey marked this conversation as resolved

Is the source/tools related changes missing or am I blind? XD
(They have not been added to the main repo)

Is the `source/tools` related changes missing or am I blind? XD (They have not been added to the main repo)
Author
Owner

Is the source/tools related changes missing or am I blind? XD
(They have not been added to the main repo)

I missed references in the build system and tools to rename source/tools to tools, it is fixed now.

The tools are in the main repository, you can see it in the https://projects.blender.org/Sergey/blender/src/branch/submodules_refactor/tools

But I indeed don't see those files in the PR. Guess here goes another bug in Gitea.

Edit: You can see those by clicking on the Some files were not shown because too many files have changed in this diff at the bottom of the Files Changed few times.

> Is the `source/tools` related changes missing or am I blind? XD > (They have not been added to the main repo) I missed references in the build system and tools to rename `source/tools` to `tools`, it is fixed now. The tools are in the main repository, you can see it in the https://projects.blender.org/Sergey/blender/src/branch/submodules_refactor/tools But I indeed don't see those files in the PR. ~~Guess here goes another bug in Gitea.~~ Edit: You can see those by clicking on the `Some files were not shown because too many files have changed in this diff` at the bottom of the `Files Changed` few times.

You can see those by clicking on the Some files were not shown because too many files have changed in this diff at the bottom of the Files Changed few times.

Ah, I missed that as well. Thanks for pointing it out! :)

> You can see those by clicking on the Some files were not shown because too many files have changed in this diff at the bottom of the Files Changed few times. Ah, I missed that as well. Thanks for pointing it out! :)

When I checkout this branch I get this message. Not expecting us to do something about it, just something to be aware of.

warning: unable to rmdir 'release/datafiles/locale': Directory not empty
warning: unable to rmdir 'release/scripts/addons_contrib': Directory not empty
warning: unable to rmdir 'source/tools': Directory not empty

Then I did make update and got this, because I was in a worktree directory instead of the original directory.

NotADirectoryError: [Errno 20] Not a directory: '/home/brecht/dev/worktree/.git/modules/release/scripts/addons'

I guess it is reasonable to just print a warning for that case, for advanced users that are using worktrees?

When I checkout this branch I get this message. Not expecting us to do something about it, just something to be aware of. ``` warning: unable to rmdir 'release/datafiles/locale': Directory not empty warning: unable to rmdir 'release/scripts/addons_contrib': Directory not empty warning: unable to rmdir 'source/tools': Directory not empty ``` Then I did `make update` and got this, because I was in a worktree directory instead of the original directory. ``` NotADirectoryError: [Errno 20] Not a directory: '/home/brecht/dev/worktree/.git/modules/release/scripts/addons' ``` I guess it is reasonable to just print a warning for that case, for advanced users that are using worktrees?
Brecht Van Lommel requested changes 2023-02-15 15:06:52 +01:00
@ -206,0 +269,4 @@
print(f"Copying {old_submodule_relative_dir} to scripts/{directory_name} ...")
old_submodule_dir = blender_git_root / old_submodule_relative_dir
shutil.copytree(old_submodule_dir, external_dir)

I was expecting this to move rather than copy. Now the directory stays behind. If we wanted to do that it should be git ignored at least? I guess also for bisect.

I was expecting this to move rather than copy. Now the directory stays behind. If we wanted to do that it should be git ignored at least? I guess also for bisect.
Sergey marked this conversation as resolved
Brecht Van Lommel approved these changes 2023-02-15 15:43:28 +01:00
Brecht Van Lommel left a comment
Owner

This seems to be working fine for me now, including worktrees and going back and forth with older branches.

Still probably someone else who knows how to recover from Git problems should try this.

This seems to be working fine for me now, including worktrees and going back and forth with older branches. Still probably someone else who knows how to recover from Git problems should try this.
@ -52,0 +64,4 @@
/release/scripts/addons
/release/datafiles/locale/
/release/scripts/addons_contrib/
/source/tools/

As far as I can tell the trailing slashes make no difference, but maybe best to leave them out for consistency.

As far as I can tell the trailing slashes make no difference, but maybe best to leave them out for consistency.
Author
Owner

We quickly talked about this with Brecht. Decided to add them everywhere instead. Seems to be that in the current state of .gitignore we use the trailing slash more than we don't :)

We quickly talked about this with Brecht. Decided to add them everywhere instead. Seems to be that in the current state of .gitignore we use the trailing slash more than we don't :)
Sergey marked this conversation as resolved
Sergey Sharybin requested review from Campbell Barton 2023-02-15 15:47:32 +01:00
Sergey Sharybin requested review from Bastien Montagne 2023-02-15 15:47:45 +01:00
Sergey Sharybin requested review from Ray molenkamp 2023-02-15 15:47:45 +01:00
Author
Owner

Still probably someone else who knows how to recover from Git problems should try this.

Indeed! Added everyone else as a reviewer who's input will be very welcome here.

> Still probably someone else who knows how to recover from Git problems should try this. Indeed! Added everyone else as a reviewer who's input will be very welcome here.
Ray molenkamp approved these changes 2023-02-15 16:27:15 +01:00
Ray molenkamp left a comment
Member

had the same warnings brecht had, which may lead to questions from some users (but not worth trying to suppress imho), but beyond that remarkably smooth transition by just running make update

had the same warnings brecht had, which may lead to questions from some users (but not worth trying to suppress imho), but beyond that remarkably smooth transition by just running `make update`
Campbell Barton approved these changes 2023-02-16 06:17:29 +01:00
Campbell Barton left a comment
Owner

Generally seems fine, tests/python/bl_keymap_completeness.py:8 needs "release", "scripts" to be replaced with "scripts".

Generally seems fine, `tests/python/bl_keymap_completeness.py:8` needs `"release", "scripts"` to be replaced with `"scripts"`.

Some notes:

  • After this change it's no longer possible to symlink release/ into {BUILD_DIR}/bin/{MAJOR_VER}.{MINOR_VER}/ to bypass the install target. While I don't see this as an argument to keep scripts/ under release, it may be worth mentioning (IIRC @JulianEisel uses/used this trick too).

  • Quite a few scripts within tools/ will need updating (searching for __file__) shows these cases. Not an issue with this PR, just something to do once it's applied.

Some notes: - After this change it's no longer possible to symlink `release/` into `{BUILD_DIR}/bin/{MAJOR_VER}.{MINOR_VER}/` to bypass the `install` target. While I don't see this as an argument to keep `scripts/` under release, it may be worth mentioning (IIRC @JulianEisel uses/used this trick too). - Quite a few scripts within `tools/` will need updating (searching for `__file__`) shows these cases. Not an issue with this PR, just something to do once it's applied.

Sorry, misclicked.

  • After this change it's no longer possible to symlink release/ into {BUILD_DIR}/bin/{MAJOR_VER}.{MINOR_VER}/ to bypass the install target. While I don't see this as an argument to keep scripts/ under release, it may be worth mentioning (IIRC @JulianEisel uses/used this trick too).

Symlinking just the scripts folder instead of the entire release folder could still be made to work?

Sorry, misclicked. > - After this change it's no longer possible to symlink `release/` into `{BUILD_DIR}/bin/{MAJOR_VER}.{MINOR_VER}/` to bypass the `install` target. While I don't see this as an argument to keep `scripts/` under release, it may be worth mentioning (IIRC @JulianEisel uses/used this trick too). Symlinking just the scripts folder instead of the entire release folder could still be made to work?
Brecht Van Lommel reopened this pull request 2023-02-16 09:53:05 +01:00

Sorry, misclicked.

  • After this change it's no longer possible to symlink release/ into {BUILD_DIR}/bin/{MAJOR_VER}.{MINOR_VER}/ to bypass the install target. While I don't see this as an argument to keep scripts/ under release, it may be worth mentioning (IIRC @JulianEisel uses/used this trick too).

Symlinking just the scripts folder instead of the entire release folder could still be made to work?

The scripts and release/datafiles are all that are needed, it's a bit awkward to link in from different directories (perhaps datafiles could be moved into the root directory too), worst case I'll make a script that sets up symlinks.

> Sorry, misclicked. > > > - After this change it's no longer possible to symlink `release/` into `{BUILD_DIR}/bin/{MAJOR_VER}.{MINOR_VER}/` to bypass the `install` target. While I don't see this as an argument to keep `scripts/` under release, it may be worth mentioning (IIRC @JulianEisel uses/used this trick too). > > Symlinking just the scripts folder instead of the entire release folder could still be made to work? The `scripts` and `release/datafiles` are all that are needed, it's a bit awkward to link in from different directories (perhaps `datafiles` could be moved into the root directory too), worst case I'll make a script that sets up symlinks.
Sergey Sharybin force-pushed submodules_refactor from f87e62cae3 to 90760f6e2b 2023-02-21 14:36:16 +01:00 Compare
Author
Owner

@blender-bot build

@blender-bot build
Author
Owner

@blender-bot build

@blender-bot build
Sergey changed target branch from main to blender-v3.5-release 2023-02-21 15:30:28 +01:00
Sergey Sharybin merged commit 03806d0b67 into blender-v3.5-release 2023-02-21 16:40:14 +01:00
Sergey Sharybin deleted branch submodules_refactor 2023-02-21 16:40:14 +01:00
Bastien Montagne removed this from the Core project 2023-07-03 12:49:16 +02:00
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#104755
No description provided.