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

SCons: Remove code for unsupported compilers #99425

Closed
wants to merge 1 commit into from

Conversation

dustdfg
Copy link
Contributor

@dustdfg dustdfg commented Nov 19, 2024

Follow up to: #99217 and #98842

@dustdfg dustdfg requested a review from a team as a code owner November 19, 2024 13:32
@dustdfg dustdfg force-pushed the remove_old_compiler_code branch from 149b253 to c264100 Compare November 19, 2024 13:54
@dustdfg
Copy link
Contributor Author

dustdfg commented Nov 19, 2024

There is also one place where VCINSTALLDIR is used. I don't know much about VS so have a question. It is defined for all VS versions or only for VS >= 2017 VS <= 2017?

https://github.com/godotengine/godot/blob/c264100da2d021b3d107f18f5adf74bdaeb60547/platform/windows/detect.py#L51-L52

@dustdfg dustdfg changed the title Remove code for unsupported compilers SCons: Remove code for unsupported compilers Nov 19, 2024
@Repiteo
Copy link
Contributor

Repiteo commented Nov 19, 2024

Oop, should've specified, the fix I showed extended to the rest of the conditional; you'll have to remove lines 120-123 as well

@dustdfg
Copy link
Contributor Author

dustdfg commented Nov 19, 2024

Oop, should've specified, the fix I showed extended to the rest of the conditional; you'll have to remove lines 120-123 as well

Already was checking for correctness. Pressing "commit changes" and then checking out to pr's head is the easiest way to get the changes in local which I know... Btw thanks for clarifying! 🙂

Follow up to: godotengine#99217 and godotengine#98842

Signed-off-by: Yevhen Babiichuk (DustDFG) <dfgdust@gmail.com>
Co-authored-by: Thaddeus Crews <repiteo@outlook.com>
@dustdfg dustdfg force-pushed the remove_old_compiler_code branch from 19447aa to 6e6661f Compare November 19, 2024 18:31
@Repiteo
Copy link
Contributor

Repiteo commented Nov 19, 2024

It also seems that VCINSTALLDIR indeed is on later VS versions, or at least on my VS2022 version. I assume then that the later check is more thorough and/or VCTOOLSINSTALLDIR wasn't introduced until VS2017.

@dustdfg
Copy link
Contributor Author

dustdfg commented Nov 19, 2024

It also seems that VCINSTALLDIR indeed is on later VS versions, or at least on my VS2022 version. I assume then that the later check is more thorough and/or VCTOOLSINSTALLDIR wasn't introduced until VS2017.

Looks like I understood words but honestly I couldn't parse it. Looks like I've already got debuff on mind capabilities 😅 Could you rephrase please?

@Repiteo
Copy link
Contributor

Repiteo commented Nov 19, 2024

Short version: it could still be used. So while legacy, it's not necessarily unsupported, and should probably be kept as-is for now. Maybe this PR could be refactored to just have the SConstruct change

@dustdfg
Copy link
Contributor Author

dustdfg commented Nov 20, 2024

It is not so much sense to merge deletion of two lines of stale code which won't create any problems. I will glue it with some other misc edits

@dustdfg dustdfg closed this Nov 20, 2024
@dustdfg dustdfg deleted the remove_old_compiler_code branch November 20, 2024 07:51
@AThousandShips AThousandShips removed this from the 4.4 milestone Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants