Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Wrong UserAgent for windows 10 as Compatibility Manifest were not getting applied in windows build #681

Merged
merged 1 commit into from Mar 31, 2020
Merged

Wrong UserAgent for windows 10 as Compatibility Manifest were not getting applied in windows build #681

merged 1 commit into from Mar 31, 2020

Conversation

niteskum
Copy link
Contributor

Brackets windows version compatibility configuration are added in compatibility.manifest file,
but this compatibility configuration were not getting applied in build as this "compatibility.manifest" was not added in "AdditionalManifestFiles".

Added "compatibility.manifest" file in "AdditionalManifestFiles" , Now correct UserAgent string are coming for windows 10.

@narayani28 @jha-G please review.

Copy link
Collaborator

@narayani28 narayani28 left a comment

Choose a reason for hiding this comment

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

Can you add reference links for the syntax added for the new file?

@niteskum
Copy link
Contributor Author

niteskum commented Mar 30, 2020

Can you add reference links for the syntax added for the new file?

I just checked how it is added in other open source project(e.g chromium) and used same syntax :
https://chromium.googlesource.com/chromium/chromium/+/trunk/build/common.gypi#5368

Copy link
Collaborator

@narayani28 narayani28 left a comment

Choose a reason for hiding this comment

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

Should you check for valid manifest file similar to " ['_type=="executable" and ">(win_exe_compatibility_manifest)"!=""', " as mentioned in the reference link?

@niteskum
Copy link
Contributor Author

Should you check for valid manifest file similar to " ['_type=="executable" and ">(win_exe_compatibility_manifest)"!=""', " as mentioned in the reference link?

in reference link they only have compatibility_manifest. but in our case we have two manifest so if "win_exe_compatibility_manifest" is empty ,we still like to continue with just one manifest (appshell.exe.manifest). having empty variable "win_exe_compatibility_manifest" will not cause any issue.

@narayani28
Copy link
Collaborator

have you verified that if the compatibility.manifest didnt exist on the machine, does the build succeed and launch?

@niteskum
Copy link
Contributor Author

have you verified that if the compatibility.manifest didnt exist on the machine, does the build succeed and launch?

if compatibility.manifest does not exist or does not have right permission or invalid, build will fail. I don't see there is anyway we can have check for existence and validity of a file in config .

at Line no 93 : we set "win_exe_compatibility_manifest" variable to 'compatibility.manifest'. what I verified that if I set variable 'win_exe_compatibility_manifest' to empty , build succeed and launch.

in reference link also check was if 'win_exe_compatibility_manifest' not empty

@narayani28
Copy link
Collaborator

LGTM

@narayani28 narayani28 merged commit 477baba into adobe:master Mar 31, 2020
narayani28 added a commit that referenced this pull request Mar 31, 2020
Merge pull request #681 from niteskum/compatibilityManifestNotAppliedBug
@niteskum
Copy link
Contributor Author

@nethip Could you please review this PR?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants