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

fix(163): Update zenject.asmdef to specify all default values #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NoxMortem
Copy link

@NoxMortem NoxMortem commented Aug 18, 2020

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • No compiler errors or warnings

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Issue Number

Issue Number: #163

What is the current behavior?

  • zenject.asmdef is not recognized by unity inspector and therefore the project cannot be used.

What is the new behavior?

zenject.asmdef is recognized by unity inspector and the project therefore can be referenced and used.

Does this introduce a breaking change?

  • Yes
  • No
  • Maybe: It is unclear if all versions of unity that zenject might support treat the asmdef the same and if older versions might have been able to read the old asmdef but newer versions don't. This would be a breaking change introduced by unity.

Other information

grafik
grafik

On which Unity version has this been tested?

  • 2020.4 LTS
  • 2020.3
  • 2020.2.1f1
  • 2020.1
  • 2019.4 LTS
  • 2019.3
  • 2019.2
  • 2019.1
  • 2018.4 LTS

Scripting backend:

  • Mono
  • IL2CPP

Note: Every pull request is tested on the Continuous Integration (CI) system to confirm that it works in Unity.

Ideally, the pull request will pass ("be green"). This means that all tests pass and there are no errors. However, it is not uncommon for the CI infrastructure itself to fail on specific platforms or for so-called "flaky" tests to fail ("be red"). Each CI failure must be manually inspected to determine the cause.

CI starts automatically when you open a pull request, but only Releasers/Collaborators can restart a CI run. If you believe CI is giving a false negative, ask a Releaser to restart the tests.

@NoxMortem NoxMortem force-pushed the bugfix/162_UpdateZenjectAsmdef branch from 8f9dafb to 817e4ec Compare August 18, 2020 10:32
@NoxMortem
Copy link
Author

NoxMortem commented Aug 18, 2020

Fixed build error by replacing zenject.ref.asmref with Zenject.SampleBuilder.Editor.asmdef which is only included in the editor to avoid usage of UnityEditor outside of the editor. Build project (win x64, 2020.1.2f1) to ensure it builds.
grafik

@NoxMortem NoxMortem force-pushed the bugfix/162_UpdateZenjectAsmdef branch from 817e4ec to ca06f46 Compare August 18, 2020 10:41
@NoxMortem
Copy link
Author

Reset project to 2019.3.0

@NoxMortem
Copy link
Author

... And by resetting the project version I've lost the actual fix.

…ef which are required if Zenject.asmdef is not using auto reference (+2 squashed commit)

[86a08a93] fix: Re-add Zenject.asmdef and rename to "Z"enject as the assembly should match the name for case sensitive systems.
[ca06f46] fix: Reset project version to 2019.3.0f1 and remove all related changes (+1 squashed commits)
[817e4ec] fix(build): Replace zenject.ref.asmref with Zenject.SampledBuilder.Editor.asmdef which is only included in the editor to avoid usage of UnityEditor outside of the editor (+1 squashed commits)
[8f9dafb] fix(.gitignore): Ignore UnityProject/UserSettings/* which is a new user specific folder, which previously was stored in /Library (see https://forum.unity.com/threads/whats-the-usersettings-directory.754436/#post-5029937)
fix(packages-lock.json): Should be checked in, but wasn't or did not exist in the old unity version (+1 squashed commits)
[1f431e3] fix(161): Update zenject.asmdef to specify all default values
@NoxMortem NoxMortem force-pushed the bugfix/162_UpdateZenjectAsmdef branch from ca06f46 to bd8ef11 Compare August 18, 2020 11:36
@Mathijs-Bakker
Copy link
Collaborator

This is a known Unity issue. Simply put: they messed up the assembly definitions starting with 2019.1 and changed it's behavior with every update. 2020 is for Unity a 'FIX' year. They changed their release cycle to two releases each year. So hopefully they have this right with 2020.2/LTS.
BTW the same goes for UPM. They have taken all their devs of their feature projects. And have them working on fixes.

@NoxMortem
Copy link
Author

NoxMortem commented Aug 18, 2020

Reapplied the original changes in 2020.1.2f1 but pushed without the project version upgrade to avoid failing to build. Added .asmdef for SampleGame1 and SampleGame2 which are required if Zenject is not set to auto reference, which no asmdef ever should be that is not required to be referenced everywhere.

@Mathijs-Bakker thanks. Should this PR be declined and we wait for the Unity fix instead?

@RichardWepner
Copy link
Collaborator

The file zenject.asmdef got renamed to Zenject.asmdef, but the corresponding *.meta file wasn't renamed to Zenject.asmdef.meta accordingly. On Windows, this shouldn't make a noticeable difference, but on any other system (with case sensitive paths), this could lead Unity to detect the *.meta file as a stray one for a non existing asmdef and remove it, and regenerating the one for the actual asmdef, causing a regeneration of the GUID.
TLDR: please also rename the *.meta file to avoid issues

Regarding the Unity-behavior regarding the asmdefs: I didn't do testing specifically to test this behavior, but based on my experience with the Unity serialization, I don't think this should negatively affect projects using an older version of Unity. I might test out what Untiy 2019 does if it's presented with the altered asmdef.

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