Skip to content

Change Assembly Version to AssemblyVersion in win_pe #4116

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

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

Conversation

alok1304
Copy link
Contributor

Fixes #3790

Tasks

  • Reviewed contribution guidelines
  • PR is descriptively titled 📑 and links the original issue above 🔗
  • Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
    Run tests locally to check for errors.
  • Commits are in uniquely-named feature branch and has no merge conflicts 📁
  • Updated documentation pages (if applicable)
  • Updated CHANGELOG.rst (if applicable)

Signed-off-by: Alok Kumar [email protected]

@alok1304 alok1304 force-pushed the fix/assembly-version-format branch from 9cc045f to 51c9005 Compare January 23, 2025 15:39
@alok1304
Copy link
Contributor Author

alok1304 commented Jan 23, 2025

@pombredanne two test cases are failing when I used AssemblyVersion for
test_win_pe_Moq_Silverlight_dll and
test_win_pe_microsoft_practices_enterpriselibrary_caching_dll in test_win_pe.py

it shows (I checked for test_win_pe_Moq_Silverlight_dll)

>       assert result == expected
E       AssertionError: assert {'AssemblyVer...y': None, ...} == {'AssemblyVer...y': None, ...}
E
E         Omitting 20 identical items, use -vv to show
E         Differing items:
E         {'extra_data': {'Assembly Version': '4.2.1507.118'}} != {'extra_data': {}}
E         Use -v to get more diff

it shows that the result dictionary contains an extra_data field with {'Assembly Version': '4.2.1507.118'} that means extra_data field contains Assembly Version.

I think no need to change this 'Assembly Version' to 'AssemblyVersion' because extra_data field already contains
Assembly Version.

I think we should not handle the correct Windows PE versions when we using AssemblyVersion in #3790.
@pombredanne Am I Right??

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both "Assembly Version" and "AssemblyVersion" exist in the wild. See the your updated tests that are skipping this value now.
This is not correct:
https://github.com/aboutcode-org/scancode-toolkit/pull/4116/files#diff-912e6b6b9ff4c33ea88d4f914e955f723fb024e17161592bfc2a2124718db218L5

You cannot replace one with the other, if need be you need both, if you can find a tiny test PE that actually contains "AssemblyVersion".
See https://github.com/search?q=["Assembly+Version"]+language%3AC%23&type=code

@alok1304 alok1304 force-pushed the fix/assembly-version-format branch from 0c185f0 to 905a6cc Compare March 30, 2025 13:53
@alok1304 alok1304 force-pushed the fix/assembly-version-format branch from 905a6cc to 9caa6b3 Compare March 30, 2025 15:39
@alok1304
Copy link
Contributor Author

Hey @pombredanne

You cannot replace one with the other, if need be you need both, if you can find a tiny test PE that actually contains "AssemblyVersion".

I use both "Assembly Version" and "AssemblyVersion" Now it is working and also all test cases are passed.

@alok1304 alok1304 requested a review from pombredanne March 30, 2025 16:10
@AyanSinhaMahapatra
Copy link
Member

@alok1304 see #4116 (review) you need to add a minimal test with a real example to test this functionality. Look for test files similar to what we have, do you need any help finding these?

@alok1304
Copy link
Contributor Author

alok1304 commented Apr 12, 2025

Look for test files similar to what we have, do you need any help finding these?

yes, I need help, how do we find these files for the test that contains AssemblyVersion?

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.

Check that we handle correctly the Windows PE versions
3 participants