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

Remove unnecessary macros #159

Open
llxia opened this issue Mar 11, 2021 · 8 comments
Open

Remove unnecessary macros #159

llxia opened this issue Mar 11, 2021 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@llxia
Copy link
Contributor

llxia commented Mar 11, 2021

Due to historical reasons, we have D, SQ, and Q macros defined in makefile in TKG and used in test repos.

For example:
https://github.com/AdoptOpenJDK/TKG/blob/master/compile.mk#L17
https://github.com/AdoptOpenJDK/TKG/blob/master/makeGen.mk#L26
https://github.com/AdoptOpenJDK/TKG/blob/master/makeGen.mk#L28

It will be more clear and simple if these values can be explicitly written in makefile and test files. For example,

  • we do not need to modify the value when copying from the playlist for running system_custom
  • macro is not defined issue will not happen

I think we should verify and remove the macros from makefiles and test repos if they are unnecessary.

@karianna karianna added this to To do in TKG via automation Mar 18, 2021
@karianna karianna added the bug Something isn't working label Mar 18, 2021
@2001asjad
Copy link

Hi @llxia , can you tell more about this issue like what files to look on and how to approach etc.
Thank You
PS: I am an Outreachy applicant

@llxia
Copy link
Contributor Author

llxia commented Apr 19, 2021

Thanks @2001asjad . Using D as an example, we need to remove D in TKG repo and replace ${D} with / in other repos (i.e., AdoptOpenJDK/test, OpenJ9, etc)

@2001asjad
Copy link

Ok, I'll work on this issue and we can discuss more of this after I make a PR.
Thank you @llxia

@smlambert
Copy link
Contributor

@llxia @renfeiw - for this issue, is it preferred to create a separate makefile for the common macros (D, SQ, Q), that can then be used by all test makefiles (through the use of include statements, see https://stackoverflow.com/questions/19307258/how-to-include-a-makefile-into-an-other-makefile).

@2001asjad - It will be useful for you to search through all of the .mk files in this repo as a starting point and see where those D, SQ and Q variables are defined. You will find that they are defined in multiple places. I believe the preference is to define them in 1 place (perhaps a new file called common.mk) and then include them in the files where they are used.

@karianna karianna moved this from To do to In progress in TKG Apr 20, 2021
@karianna karianna added this to the April 2021 milestone Apr 20, 2021
@2001asjad
Copy link

2001asjad commented Apr 20, 2021

@llxia @renfeiw - for this issue, is it preferred to create a separate makefile for the common macros (D, SQ, Q), that can then be used by all test makefiles (through the use of include statements, see https://stackoverflow.com/questions/19307258/how-to-include-a-makefile-into-an-other-makefile).

@2001asjad - It will be useful for you to search through all of the .mk files in this repo as a starting point and see where those D, SQ and Q variables are defined. You will find that they are defined in multiple places. I believe the preference is to define them in 1 place (perhaps a new file called common.mk) and then include that common.mk file in the files where the variables are used.

Ok thank you @smlambert I'll try working on this, if I get stuck I'll ask it then only

@renfeiw
Copy link
Contributor

renfeiw commented Apr 20, 2021

@llxia @renfeiw - for this issue, is it preferred to create a separate makefile for the common macros (D, SQ, Q), that can then be used by all test makefiles (through the use of include statements, see https://stackoverflow.com/questions/19307258/how-to-include-a-makefile-into-an-other-makefile).

Yes, that's exactly what I thought how we can enhance this.

@llxia
Copy link
Contributor Author

llxia commented Apr 20, 2021

Yes, I agree. I think it is easier this way.

@2001asjad
Copy link

Hello @smlambert , so I came up with a solution, I can make a new makefile where I'll define D, Q, P etc and remove those from rest of the files and use them in those file via importing them.
It's like as you mentioned above

@karianna karianna modified the milestones: April 2021, May 2021 May 4, 2021
@smlambert smlambert removed this from the May 2021 milestone Jun 2, 2021
@karianna karianna removed this from In progress in TKG Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants